chore(deps): update sqllogictest requirement from 0.23.0 to 0.24.0 #13902
Conversation
xudong963
force-pushed
the
sqllogictest-dep
branch
from
bae5110 to
15c4eac
Compare
| } | ||
|
|
||
| fn value_validator( | ||
| normalizer: Normalizer, |
There was a problem hiding this comment.
Not related to this PR, I'm a bit confused about why sqllogictests need a Normalizer🤔
The value validator can already be customized, and specifically, the validator will call the normalizer within its proces. Normalizer seems like an unnecessary design to me.
There was a problem hiding this comment.
I think @Omega359 added this in
He may have more backstory as well
There was a problem hiding this comment.
Not related to this PR, I'm a bit confused about why sqllogictests need a
Normalizer🤔 The value validator can already be customized, and specifically, the validator will call the normalizer within its proces.Normalizerseems like an unnecessary design to me.
That was not entirely true unfortunately, the normalization was hardcoded in places.
There was a problem hiding this comment.
That was not entirely true unfortunately, the normalization was hardcoded in places.
normalize_string was an internal part of the default validator. Although it was hard-coded, the outer validator can be user-defined. A user-defined validator like the one provided by DataFusion can offer more flexibility to meet specific row comparison needs, and might not require normalization.
There was a problem hiding this comment.
Yes. However, in my sidebranch for the sqlite tests it made things a bit cleaner, and in the branch I used to generate the 'cleansed' .slt files it was necessary as normalization was required where validation was not. I have no desire to spend any more time working on that dependency tbh so if you feel strongly about it I'll point you at that project :)
There was a problem hiding this comment.
Thank you @Omega359 for the explanation. I don't have a strong opinion on this, just curious about its use cases. Since it can make some things cleaner, its existence is reasonable.
Which issue does this PR close?
Closes ##13884
Rationale for this change
What changes are included in this PR?
Resolve the breaking change ("The Validator type used in various function in Runner implementation has a new required field Normalizer that is used to normalize result values.")
Are these changes tested?
Are there any user-facing changes?