Skip to content

fix vector grouping expression deferred evaluation to only consider dictionary encoded strings as fixed width #16666

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.

Sign up for GitHub

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

Merged
clintropolis merged 3 commits into from
Jun 27, 2024
Merged

fix vector grouping expression deferred evaluation to only consider dictionary encoded strings as fixed width #16666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48fc029
fix vector grouping expression deferred evaluation to only consider d…
clintropolis Jun 27, 2024
eb044c5
tests
clintropolis Jun 27, 2024
783eb64
fix strange
clintropolis Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions processing/src/main/java/org/apache/druid/query/groupby/DeferExpressionDimensions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public boolean useDeferredGroupBySelector(
return false;
}

if (!capabilities.isNumeric() && !capabilities.isDictionaryEncoded().isTrue()) {
if (!capabilities.isNumeric() && !isDictionaryEncodedScalarString(capabilities)) {
// Not fixed-width.
return false;
}
Expand Down Expand Up @@ -106,7 +106,7 @@ public boolean useDeferredGroupBySelector(

allNumericInputs = allNumericInputs && capabilities.isNumeric();

if (!capabilities.isNumeric() && !capabilities.isDictionaryEncoded().isTrue()) {
if (!capabilities.isNumeric() && !isDictionaryEncodedScalarString(capabilities)) {
// Not fixed-width.
return false;
}
Expand Down Expand Up @@ -162,6 +162,25 @@ public String toString()
return jsonName;
}


/**
* {@link VectorColumnSelectorFactory} currently can only make dictionary encoded selectors for string types, so
* we can only consider them as fixed width. Additionally, to err on the side of safety, multi-value string columns
* are also not considered fixed width because expressions process multi-value dimensions as single rows, so we would
* need all dictionary ids to be present in the combined key.
*
* At the time of this javadoc, vector group by does not support multi-value dimensions anyway, so this isn't really
* a problem, but if it did, we could consider allowing them if we ensure that all multi-value inputs are used as
* scalars and so the expression can be applied separately to each individual dictionary id (e.g. the equivalent of
* {@link ExpressionPlan.Trait#SINGLE_INPUT_MAPPABLE} but for all multi-value string inputs of the expression).
*/
private static boolean isDictionaryEncodedScalarString(ColumnCapabilities capabilities)
{
return capabilities.isDictionaryEncoded().isTrue() &&
capabilities.is(ValueType.STRING) &&
capabilities.hasMultipleValues().isFalse();
}

/**
* Whether the given expression can be deferred innately by the selector created by
* {@link ExpressionVirtualColumn#makeSingleValueVectorDimensionSelector(DimensionSpec, VectorColumnSelectorFactory)}.
Expand Down
Loading