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 2022/10/26 20:30:43 UTC
[GitHub] [druid] clintropolis opened a new pull request, #13267: fix nested column thread safety issue
clintropolis opened a new pull request, #13267:
URL: https://github.com/apache/druid/pull/13267
smaller footprint version of fix #13265 for 24.0.1, this just fixes the `TypeStrategy` to use positional reads
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #13267:
URL: https://github.com/apache/druid/pull/13267#issuecomment-1295584079
> it looks like integration tests are still failing in the 24.0.1 branch. We might want to check if there is something non-deterministic or not.
hmm, these actually look unrelated to my fix, but maybe worth looking into
```
[ERROR] org.apache.druid.sql.avatica.DruidAvaticaHandlerTest.testConcurrentQueries Time elapsed: 12.981 s <<< ERROR!
java.util.concurrent.ExecutionException: java.lang.RuntimeException: org.apache.calcite.avatica.AvaticaSqlException: Error -1 (00000) : Error while executing SQL "SELECT COUNT(*) + 1636 AS ci FROM foo": Remote driver error: QueryInterruptedException: Error while applying rule DruidQueryRule(AGGREGATE_PROJECT), args [rel#631993:LogicalProject.NONE.[](input=RelSubset#631992,ci=+($0, 1636)), rel#632012:DruidQueryRel.NONE.[](query={"queryType":"timeseries","dataSource":{"type":"table","name":"foo"},"intervals":{"type":"intervals","intervals":["-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z"]},"granularity":{"type":"all"},"aggregations":[{"type":"count","name":"a0"}],"context":{"nativeQueryIds":"[3ef1e7e1-f2f6-4152-8384-229eaea39482]","sqlQueryId":"f2727102-ff6c-404c-8f9a-2ed84a406e46","sqlStringifyArrays":false}},signature={a0:LONG})] -> RuntimeException: Error while applying rule DruidQueryRule(AGGREGATE_PROJECT), args [rel#631993:LogicalProject.NONE.[](input=RelSubset#
631992,ci=+($0, 1636)), rel#632012:DruidQueryRel.NONE.[](query={"queryType":"timeseries","dataSource":{"type":"table","name":"foo"},"intervals":{"type":"intervals","intervals":["-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z"]},"granularity":{"type":"all"},"aggregations":[{"type":"count","name":"a0"}],"context":{"nativeQueryIds":"[3ef1e7e1-f2f6-4152-8384-229eaea39482]","sqlQueryId":"f2727102-ff6c-404c-8f9a-2ed84a406e46","sqlStringifyArrays":false}},signature={a0:LONG})] -> NullPointerException: Cannot invoke "java.util.Map.size()" because "m" is null
```
I think maybe https://github.com/apache/druid/pull/13049 is the fix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13267:
URL: https://github.com/apache/druid/pull/13267#discussion_r1007496388
##########
processing/src/main/java/org/apache/druid/segment/NestedDataColumnMerger.java:
##########
@@ -163,13 +162,8 @@ private GlobalDictionarySortedCollector getSortedIndexFromIncrementalAdapter(
return null;
}
final NestedDataColumnIndexer indexer = (NestedDataColumnIndexer) dim.getIndexer();
- for (Map.Entry<String, NestedDataColumnIndexer.LiteralFieldIndexer> entry : indexer.fieldIndexers.entrySet()) {
- // skip adding the field if no types are in the set, meaning only null values have been processed
- if (!entry.getValue().getTypes().isEmpty()) {
- mergedFields.put(entry.getKey(), entry.getValue().getTypes());
- }
- }
- return indexer.globalDictionary.getSortedCollector();
+ indexer.mergeFields(mergedFields);
+ return indexer.getSortedCollector();
Review Comment:
yep, made writing the test easier and the direct field access was kind of ugly
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13267:
URL: https://github.com/apache/druid/pull/13267#discussion_r1007467952
##########
processing/src/main/java/org/apache/druid/segment/NestedDataColumnMerger.java:
##########
@@ -163,13 +162,8 @@ private GlobalDictionarySortedCollector getSortedIndexFromIncrementalAdapter(
return null;
}
final NestedDataColumnIndexer indexer = (NestedDataColumnIndexer) dim.getIndexer();
- for (Map.Entry<String, NestedDataColumnIndexer.LiteralFieldIndexer> entry : indexer.fieldIndexers.entrySet()) {
- // skip adding the field if no types are in the set, meaning only null values have been processed
- if (!entry.getValue().getTypes().isEmpty()) {
- mergedFields.put(entry.getKey(), entry.getValue().getTypes());
- }
- }
- return indexer.globalDictionary.getSortedCollector();
+ indexer.mergeFields(mergedFields);
+ return indexer.getSortedCollector();
Review Comment:
This change is just general refactoring and not actually functionally meaningful, right? Just validating my understanding
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
xvrl commented on PR #13267:
URL: https://github.com/apache/druid/pull/13267#issuecomment-1295504786
@clintropolis it looks like integration tests are still failing in the 24.0.1 branch. We might want to check if there is something non-deterministic or not.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged PR #13267:
URL: https://github.com/apache/druid/pull/13267
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #13267: fix nested column thread safety issue for 24.0.1
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #13267:
URL: https://github.com/apache/druid/pull/13267#issuecomment-1295591488
created #13281 to backport that fix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org