Fallback on null empty value in ExprBoundaries::try_from_column#8501
Conversation
razeghi71
force-pushed
the
fix/interval-expr-boundries-fall-back-on-null
branch
from
67d7872 to
1d942b7
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @razeghi71 -- I think this PR is acceptable to get the queries running, though I think implementing ScalarValue::Map is likely the more robust solution
To merge this PR I think it needs some sort of test (ideally a query in https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest) that fails without this PR and works with it. This will ensure we don't accidentally break the system again during a future refactor
Thanks again for your help here
| info!("Registering table with many types"); | ||
| register_table_with_many_types(test_ctx.session_ctx()).await; | ||
| } | ||
| "explain.slt" => { |
There was a problem hiding this comment.
Since I couldn't find a SQL syntax to create a table containing a map data type, I created it here in code.
razeghi71
force-pushed
the
fix/interval-expr-boundries-fall-back-on-null
branch
from
e9c23ab to
a700c2a
Compare
|
Thanks @alamb I added a test in |
razeghi71
force-pushed
the
fix/interval-expr-boundries-fall-back-on-null
branch
from
a700c2a to
6e80832
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @razeghi71
|
Upon some more thought I think the test might be easier to find in |
2 participants
Which issue does this PR close?
Closes #8262
Rationale for this change
Previously in c9330bc we changed the default empty value for lower and upper bound from
ScalarValue::NulltoScalarValue::try_from(field.data_type()). but the drawback of this was that for the types that this wasn't implemented (as mentioned in the issue forMap), this cause the queries to fail. To fix this we can either implement this for other data types (for map I did a prototype in #8488), or fall back onNullwhich I did in this PR.What changes are included in this PR?
Explained above.
Are these changes tested?
I tested it using the query provided in the issue.
Are there any user-facing changes?
No