Repository navigation
fix(proto): preserve case_insensitive of SimilarTo across a round trip - #26832
Open
0xdeafbeef wants to merge 1 commit into
Open
0xdeafbeef wants to merge 1 commit into
0xdeafbeef wants to merge 1 commit into
Conversation
… trip `SimilarToNode` had no field for `case_insensitive`, so `datafusion-proto` encoded an `Expr::SimilarTo` without it and decoded it as `false`. The physical planner runs a case-insensitive `SimilarTo` as a case-insensitive regex match, so after a round trip the expression matched case-sensitively. `SimilarToNode` gets `bool case_insensitive = 5`. A payload without the field decodes as `false`, as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Found while fixing the SQL unparser's handling of
SIMILAR TOin #26831; no separate issue.Rationale for this change
After a logical plan round trip through
datafusion-proto, a case-insensitiveSimilarTomatches case-sensitively.SimilarToNodehad no field forcase_insensitive.The encoder ignored the flag, and the decoder passed
false.The physical planner runs an
Expr::SimilarTowithcase_insensitive: trueas a case-insensitive regex match (RegexIMatch/RegexNotIMatch).The SQL planner always creates
Expr::SimilarTowithcase_insensitive: false, so only expressions built in Rust carrytrue.What changes are included in this PR?
SimilarToNodegetsbool case_insensitive = 5, whichto_protowrites andfrom_protoreads.prost.rsandpbjson.rsare regenerated with thedatafusion/proto-models/gengenerator, and the generated diff touches onlySimilarToNode.The pbjson serializer writes
caseInsensitiveonly when it istrue, so the JSON of a case-sensitiveSimilarTodoes not change.A payload without the field decodes as
false, the same result as before this PR.The other option is a separate message, as
LIKEandILIKEuseLikeNodeandILikeNode.What is the testing strategy for this PR?
roundtrip_similar_toindatafusion/proto/tests/cases/roundtrip_logical_plan.rstakes acase_insensitiveparameter and gets two cases withtrue:similar_to(false, None, true)andsimilar_to(true, Some('X'), true).Without the fix the test fails on the first of them: the decoded expression has
case_insensitive: false.I ran these checks locally, not in CI:
cargo test -p datafusion-proto --features json,avro: lib tests, doc-tests andproto_integrationpass, except seven parquet source tests that need theparquet-testingsubmodule data, which the local checkout did not have.The same seven fail without this change.
cargo fmt --all -- --check,cargo clippy -p datafusion-proto -p datafusion-proto-models --all-targets -- -D warningswith and without--all-features,typoson the changed files, and prettier on the upgrade guide pass.Are there any user-facing changes?
The generated
SimilarToNodestruct has a new public field, so code that builds it with a struct literal must setcase_insensitive.docs/source/library-user-guide/upgrading/56.0.0.mdgets a section with a before/after struct literal.This change needs the
api changelabel.