Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallback on null empty value in ExprBoundaries::try_from_column #8501

Merged

Conversation

razeghi71
Copy link
Contributor

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::Null to ScalarValue::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 for Map), 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 on Null which 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

@github-actions github-actions bot added the physical-expr Physical Expressions label Dec 11, 2023
@razeghi71 razeghi71 force-pushed the fix/interval-expr-boundries-fall-back-on-null branch from 67d7872 to 1d942b7 Compare December 11, 2023 19:42
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Dec 14, 2023
@@ -84,6 +84,10 @@ impl TestContext {
info!("Registering table with many types");
register_table_with_many_types(test_ctx.session_ctx()).await;
}
"explain.slt" => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I couldn't find a SQL syntax to create a table containing a map data type, I created it here in code.

@razeghi71 razeghi71 force-pushed the fix/interval-expr-boundries-fall-back-on-null branch from e9c23ab to a700c2a Compare December 14, 2023 23:52
@razeghi71 razeghi71 marked this pull request as ready for review December 14, 2023 23:52
@razeghi71
Copy link
Contributor Author

Thanks @alamb I added a test in explain.slt. Let me know if you think that's enough.

@razeghi71 razeghi71 requested a review from alamb December 15, 2023 00:00
@razeghi71 razeghi71 force-pushed the fix/interval-expr-boundries-fall-back-on-null branch from a700c2a to 6e80832 Compare December 15, 2023 00:14
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thank you @razeghi71

@alamb alamb merged commit e1a9177 into apache:main Dec 15, 2023
22 checks passed
@alamb
Copy link
Contributor

alamb commented Dec 15, 2023

Upon some more thought I think the test might be easier to find in map.slt so I proposed moving it in #8550

@razeghi71 razeghi71 deleted the fix/interval-expr-boundries-fall-back-on-null branch December 15, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter crashes when the input schema contains a map
2 participants