-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
remove legacy SQL incompatible modes #17609
remove legacy SQL incompatible modes #17609
Conversation
changes: * druid.generic.useDefaultValueForNull is no longer wired up to anything and will log.warn if set to true * druid.generic.useThreeValueLogicForNativeFilters is no longer wired up to anything and will log.warn if set to false * clean up and simplify all code and tests related to null handling configs
b4dd668
to
8ace23c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. -47k lines, lol.
Could you please look into the CodeQL failure? On https://github.com/apache/druid/pull/17609/checks?check_run_id=35290326421 it says that "alerts not introduced by this pull request might have been detected because the code changes were too large". If they were all pre-existing then we can ignore the check. I just want to make sure this patch didn't introduce new problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the CodeQL report, I see 54 "critical" or "error" items: https://github.com/apache/druid/security/code-scanning?query=pr%3A17609+tool%3ACodeQL+is%3Aopen+severity%3Acritical%2Cerror. The dates on these are all well in the past, so they appear to be pre-existing.
Based on this I am 👍 on the patch, since the rest of it looks good.
@clintropolis What is the strategy for users still using the default value for NULL? Do we have a migration guide? I imagine switching to SQL compatibility mode would change the result/behavior of queries. |
The current version of the migration guide is https://druid.apache.org/docs/latest/release-info/migration-guide/, though I'm still working on updating the docs after this PR was merged since there is some stale stuff. |
Description
changes:
Release note
todo
This PR has: