You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/04/10 03:36:04 UTC
[GitHub] [druid] clintropolis opened a new pull request #9662: fix issue
with group by limit pushdown for extractionFn, expressions, joins, etc
clintropolis opened a new pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662
### Description
This PR fixes a bug with group by limit pushdown which causes incorrect query results when grouping on string columns ... TBD.
#### simple example that illustrates the bug
given rows
```
dim1, dim2
----------
'a', 'aa'
'a', 'ab'
'a', 'ba
```
and the following query
```
SELECT
dim1, substring(dim2, 1, 1), count(*)
FROM
exampleRows
GROUP BY 1,2
LIMIT 2
```
prior to the changes in this PR the results would be
```
[['a', 'a', 2]]
```
rather than the expected:
```
[['a', 'a', 2],['a', 'b', 1]]
```
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on issue #9662: fix issue with group by
limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
jihoonson commented on issue #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#issuecomment-612351354
I restarted LGTM.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #9662: fix issue with group
by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406911065
##########
File path: processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java
##########
@@ -61,6 +61,8 @@ private DimensionHandlerUtils()
public static final ColumnCapabilities DEFAULT_STRING_CAPABILITIES =
new ColumnCapabilitiesImpl().setType(ValueType.STRING)
.setDictionaryEncoded(true)
+ .setDictionaryValuesUnique(true)
Review comment:
This doesn't seem right. The only place `DEFAULT_STRING_CAPABILITIES` is `getEffectiveCapabilities` in this same file. And looking at the cases in that method, each one seems like it should behave a bit differently. So consider deleting this constant and having the `getEffectiveCapabilities` should be generating various capabilities objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406911327
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -26,6 +26,8 @@
ValueType getType();
boolean isDictionaryEncoded();
+ IsCapable areDictionaryValuesSorted();
Review comment:
nit: IMO `Capable` is a nicer name than `IsCapable`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406907301
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -707,6 +719,33 @@ protected void putToRow(ByteBuffer key, ResultRow resultRow)
);
}
}
+
+ /**
+ * check if a collection of columns will operate correctly with {@link LimitedBufferHashGrouper} for query limit
+ * pushdown
+ */
+ private boolean canPushdownLimit(Stream<String> columnNames)
Review comment:
IMO it's more elegant if this method doesn't exist, and the caller does the allMatch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r407028104
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -331,6 +333,42 @@ public static boolean isAllSingleValueDims(
});
}
+ public static void convertRowTypesToOutputTypes(
Review comment:
Would be good to have javadocs here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r407028521
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/column/StringGroupByColumnSelectorStrategy.java
##########
@@ -148,7 +153,12 @@ private void initializeGroupingKeyV2Dimension(
@Override
public Grouper.BufferComparator bufferComparator(int keyBufferPosition, @Nullable StringComparator stringComparator)
{
- if (stringComparator == null || StringComparators.LEXICOGRAPHIC.equals(stringComparator)) {
+ final boolean canCompareInts =
+ capabilities != null &&
+ capabilities.hasBitmapIndexes() &&
Review comment:
Bitmap indexes aren't required here. I don't think it hurts too much to have in practice this check here, since in the primary case where pushdown matters (direct access to a column in a regular segment) bitmap indexes will be there. But it isn't necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r407028305
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -331,6 +333,42 @@ public static boolean isAllSingleValueDims(
});
}
+ public static void convertRowTypesToOutputTypes(
+ final List<DimensionSpec> dimensionSpecs,
+ final ResultRow resultRow,
+ final int resultRowDimensionStart
+ )
+ {
+ for (int i = 0; i < dimensionSpecs.size(); i++) {
+ DimensionSpec dimSpec = dimensionSpecs.get(i);
+ final int resultRowIndex = resultRowDimensionStart + i;
+ final ValueType outputType = dimSpec.getOutputType();
+
+ resultRow.set(
+ resultRowIndex,
+ DimensionHandlerUtils.convertObjectToType(resultRow.get(resultRowIndex), outputType)
+ );
+ }
+ }
+
+ /**
+ * check if a column will operate correctly with {@link LimitedBufferHashGrouper} for query limit pushdown
+ */
+ public static boolean canPushDownLimit(ColumnSelectorFactory columnSelectorFactory, String columnName)
+ {
+ ColumnCapabilities capabilities = columnSelectorFactory.getColumnCapabilities(columnName);
+ if (capabilities != null) {
+ // strings can be pushed down if dictionaries are sorted and unique per id
Review comment:
This comment isn't adding much, it's just repeating the couple of lines below it in English. I'd either delete it, or replace it with a comment that adds information by explaining why this fact is the case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406907933
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -555,7 +557,17 @@ public HashAggregateIterator(
final DefaultLimitSpec limitSpec = query.isApplyLimitPushDown() &&
querySpecificConfig.isApplyLimitPushDownToSegment() ?
(DefaultLimitSpec) query.getLimitSpec() : null;
+
+
+ boolean canDoPushdown = false;
Review comment:
`canDoLimitPushDown` would be a better name for two reasons:
1. There's other stuff that can be pushed down, in general, other than limits.
2. Spelling of PushDown is more consistent with the methods on GroupByQuery itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406911065
##########
File path: processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java
##########
@@ -61,6 +61,8 @@ private DimensionHandlerUtils()
public static final ColumnCapabilities DEFAULT_STRING_CAPABILITIES =
new ColumnCapabilitiesImpl().setType(ValueType.STRING)
.setDictionaryEncoded(true)
+ .setDictionaryValuesUnique(true)
Review comment:
This doesn't seem right. The only place using `DEFAULT_STRING_CAPABILITIES` is `getEffectiveCapabilities` in this same file. And looking at the cases in that method, each one seems like it should behave a bit differently. So consider deleting this constant and having the `getEffectiveCapabilities` should be generating various capabilities objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r406912419
##########
File path: processing/src/main/java/org/apache/druid/segment/join/table/IndexedTableColumnSelectorFactory.java
##########
@@ -56,6 +56,9 @@ static ColumnCapabilities columnCapabilities(final IndexedTable table, final Str
capabilities.setDictionaryEncoded(true);
}
+ capabilities.setDictionaryValuesSorted(true);
Review comment:
The dictionary value is just the row number. They aren't sorted.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #9662: fix
issue with group by limit pushdown for extractionFn, expressions, joins, etc
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9662: fix issue with group by limit pushdown for extractionFn, expressions, joins, etc
URL: https://github.com/apache/druid/pull/9662#discussion_r407028153
##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngineV2.java
##########
@@ -331,6 +333,42 @@ public static boolean isAllSingleValueDims(
});
}
+ public static void convertRowTypesToOutputTypes(
+ final List<DimensionSpec> dimensionSpecs,
+ final ResultRow resultRow,
+ final int resultRowDimensionStart
+ )
+ {
+ for (int i = 0; i < dimensionSpecs.size(); i++) {
+ DimensionSpec dimSpec = dimensionSpecs.get(i);
+ final int resultRowIndex = resultRowDimensionStart + i;
+ final ValueType outputType = dimSpec.getOutputType();
+
+ resultRow.set(
+ resultRowIndex,
+ DimensionHandlerUtils.convertObjectToType(resultRow.get(resultRowIndex), outputType)
+ );
+ }
+ }
+
+ /**
+ * check if a column will operate correctly with {@link LimitedBufferHashGrouper} for query limit pushdown
Review comment:
Would be good for these comments to be capitalized properly and have sentence-ending punctuation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org