-
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
SQL: Support GROUP BY and ORDER BY for NULL types. #16252
base: master
Are you sure you want to change the base?
Conversation
Treats SQL NULL types as strings at the native layer.
@kgyrtkirk FYI I encountered an error here in decoupled planning with one of the new tests, and disabled it: https://github.com/apache/druid/pull/16252/files#diff-8d40b0c4f1c04fc1a87f672f9fb48a91568c7bd4a4eb5ceda1c9cb008a22653fR70-R76 |
@Test | ||
public void testOrderByNullType() |
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.
instead of overriding+disabling the testcase; it can be marked as unsupported with:
@Test | |
public void testOrderByNullType() | |
@NotYetSupported(Modes.NOT_ENOUGH_RULES) | |
@Test | |
public void testOrderByNullType() |
this has the benefit that it will be verified that it still fails and could also be located based on the type of failure it have
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.
OK, thanks, cool idea!
It is supported for the basic planner, so I kept the override in DecoupledPlanningCalciteQueryTest
and changed the annotation from @Disabled
to @NotYetSupported
.
if (dataType.getSqlTypeName() == SqlTypeName.NULL) { | ||
return StringComparators.NATURAL; | ||
} else { | ||
final ColumnType valueType = getColumnTypeForRelDataType(dataType); |
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.
Should we add a new columnType called null just like the relDataType : SqlTypeName.null
In that case, we can push this logic inside getStringComparatorsForValueType() and remove this custom handling ?
thoughts ?
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.
ColumnType
is meant to represent types that the native query engine can store and process. We don't really have a pure-NULL
type there, so adding one may have larger consequences that I did not want to run into in this patch. I thought it would be better to keep the changes to the SQL layer.
This pull request has been marked as stale due to 60 days of inactivity. |
|
Treats SQL NULL types as strings at the native layer. This is consistent with how we treat unknown-type nulls in other contexts.