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/07/29 07:30:35 UTC
[GitHub] [druid] clintropolis opened a new pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
clintropolis opened a new pull request #10219:
URL: https://github.com/apache/druid/pull/10219
### Description
This PR adds a computed `isNullable` property to `ColumnCapabilities` in service of allowing segment metadata queries to accurately report which columns have null values via the updated `ColumnAnalysis`.
In a follow-up PR, with this information in place, `RowSignature` can be expanded to also track the nullable columns, allowing the SQL planner to more correctly plan a query based on whether or not a column has nulls instead of tying it to `druid.generic.useDefaultValueForNull`. `INFORMATION_SCHEMA.COLUMNS` will also correctly set `IS_NULLABLE`. This change is moderately disruptive to tests however, so I decided to split it out to keep things easy to review.
Other follow-up work to do after this PR is modifying the query engines to take advantage of this new information by examining the column capabilities instead of using the global value of `druid.generic.useDefaultValueForNull`, for example to decide if `NullableNumericAggregator` needs to be employed for numeric primitive aggregations to handle null or not.
Longer term, I would like to remove/repurpose `druid.generic.useDefaultValueForNull` and make Druid always be in SQL compatible mode, and at ingestion time just specify on the dimension specs which columns can and cannot have null values and what the default value should be (like other databases). I expect a setting will remain once the dust is settled on how to handle `null` and `''` for legacy string columns created in default mode, but I think it should be possible to transition everywhere else to just always operate in SQL compatible null handling mode with little disruption.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] 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, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] 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
---------------------------------------------------------------------
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 #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10219:
URL: https://github.com/apache/druid/pull/10219#issuecomment-666714570
the processing module unit tests are failing coverage check because the code is only run in sql compatible null handling mode (and pass the coverage check as expected for the processing tests in that mode), so I think can be ignored.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #10219: add hasNulls to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10219:
URL: https://github.com/apache/druid/pull/10219#issuecomment-673712965
CI is still failing coverage check for processing module in 'default' null handling mode because of column serde code that is only used when `druid.generic.useDefaultValueForNull=false` is set (as expected, see https://github.com/apache/druid/pull/10219#issuecomment-666714570)
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469539597
##########
File path: processing/src/main/java/org/apache/druid/segment/serde/DictionaryEncodedColumnPartSerde.java
##########
@@ -329,6 +331,7 @@ public void read(ByteBuffer buffer, ColumnBuilder builder, ColumnConfig columnCo
);
builder
.setHasMultipleValues(hasMultipleValues)
+ .setNullable(firstDictionaryEntry == null)
Review comment:
I modified the test to cover this condition, it still reports as having nulls if it ingests an empty list to a multi-value column.
##########
File path: processing/src/main/java/org/apache/druid/segment/serde/DictionaryEncodedColumnPartSerde.java
##########
@@ -329,6 +331,7 @@ public void read(ByteBuffer buffer, ColumnBuilder builder, ColumnConfig columnCo
);
builder
.setHasMultipleValues(hasMultipleValues)
+ .setNullable(firstDictionaryEntry == null)
Review comment:
I modified a column capabilities test to cover this condition, it still reports as having nulls if it ingests an empty list to a multi-value column.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469539265
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -38,6 +38,7 @@
boolean hasSpatialIndexes();
Capable hasMultipleValues();
boolean isFilterable();
+ Capable isNullable();
Review comment:
I agree, and have renamed this to `hasNulls`, but haven't changed the segment metadata `ColumnAnalysis` yet...
I view segment metadata as the computed equivalent of an SQL schema, and have exposed in with the intention of populating the `IS_NULLABLE` column in the information schema columns table, and access via `DruidSchema` will allow the SQL planner to make better plans and skip null checking and such, so I thought it still appropriate to leave this as `isNullable`. I don't feel super strongly about this though, so if you disagree I can change it.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r466140881
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -100,6 +100,11 @@ public long numRows(Segment segment)
Map<String, ColumnAnalysis> columns = new TreeMap<>();
+ Function<String, ColumnCapabilities> adapterCapabilitesFn =
Review comment:
I realize you just copied this code from somewhere else, but is there a way we can do this without `instanceof`? Maybe a new method on StorageAdapter with some nice javadocs? This code is pretty brittle otherwise.
##########
File path: processing/src/main/java/org/apache/druid/segment/serde/DictionaryEncodedColumnPartSerde.java
##########
@@ -329,6 +331,7 @@ public void read(ByteBuffer buffer, ColumnBuilder builder, ColumnConfig columnCo
);
builder
.setHasMultipleValues(hasMultipleValues)
+ .setNullable(firstDictionaryEntry == null)
Review comment:
What if it's a multi-value column with no explicit nulls, but some empty rows? Two questions in that scenario:
1. Should a column like that report "yes" or "no" for having nulls? I'm guessing "yes" makes sense, because if you filter on `col is null` then that should match empty rows. So it stands to reason that it has nulls in some sense, even though you won't see them if you walk through the column.
2. What will firstDictionaryEntry be?
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java
##########
@@ -72,22 +74,35 @@ public static ColumnCapabilitiesImpl snapshot(@Nullable final ColumnCapabilities
copy.hasMultipleValues = copy.hasMultipleValues.coerceUnknownToBoolean(unknownIsTrue);
copy.dictionaryValuesSorted = copy.dictionaryValuesSorted.coerceUnknownToBoolean(unknownIsTrue);
copy.dictionaryValuesUnique = copy.dictionaryValuesUnique.coerceUnknownToBoolean(unknownIsTrue);
+ copy.nullable = copy.nullable.coerceUnknownToBoolean(unknownIsTrue);
Review comment:
With this addition, I'm wondering if the `snapshot` method still makes sense. What reason is there that callers generally want to set all these unknown things to the same boolean? They don't seem incredibly related. Maybe we should have callers explicitly resolve all the unknowns in particular directions.
##########
File path: processing/src/main/java/org/apache/druid/segment/serde/DoubleNumericColumnPartSerdeV2.java
##########
@@ -152,13 +152,17 @@ public Deserializer getDeserializer()
buffer.position(initialPos + offset);
final ImmutableBitmap bitmap;
+ final boolean isNullable;
if (buffer.hasRemaining()) {
bitmap = bitmapSerdeFactory.getObjectStrategy().fromByteBufferWithSize(buffer);
+ isNullable = bitmap.size() > 0;
Review comment:
It would be better to use `!bitmap.isEmpty()`, because `bitmap.size()` isn't always cached and could potentially require walking the bitmap. Similar comment for the other serde types.
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -192,6 +193,7 @@ private ColumnAnalysis analyzeNumericColumn(
return new ColumnAnalysis(
capabilities.getType().name(),
capabilities.hasMultipleValues().isTrue(),
+ capabilities.isNullable().isTrue(),
Review comment:
When would it be unknown if a column has nulls?
Will bad stuff happen if we return `false` when it actually does have nulls, but for some reason it's marked as unknown?
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java
##########
@@ -685,6 +685,9 @@ IncrementalIndexRowResult toIncrementalIndexRow(InputRow row)
}
dimsKeySize += indexer.estimateEncodedKeyComponentSize(dimsKey);
// Set column capabilities as data is coming in
+ if (dimsKey == null) {
+ capabilities.setIsNullable(NullHandling.sqlCompatible());
Review comment:
If it's string won't it still count as a null, even in replace-with-default mode?
Btw, consider adding a `NullHandling.isNullable(ValueType)` method if seems useful.
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -38,6 +38,7 @@
boolean hasSpatialIndexes();
Capable hasMultipleValues();
boolean isFilterable();
+ Capable isNullable();
Review comment:
Broad comment: "hasNulls" would be a better name for this property, because it's meant to mean whether or not the column really has nulls in it right now.
"isNullable" will make people think of the SQL sense of the term, which is a different concept: it means a column could have nulls potentially, even if it doesn't right now.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469538020
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java
##########
@@ -72,22 +74,35 @@ public static ColumnCapabilitiesImpl snapshot(@Nullable final ColumnCapabilities
copy.hasMultipleValues = copy.hasMultipleValues.coerceUnknownToBoolean(unknownIsTrue);
copy.dictionaryValuesSorted = copy.dictionaryValuesSorted.coerceUnknownToBoolean(unknownIsTrue);
copy.dictionaryValuesUnique = copy.dictionaryValuesUnique.coerceUnknownToBoolean(unknownIsTrue);
+ copy.nullable = copy.nullable.coerceUnknownToBoolean(unknownIsTrue);
Review comment:
I agree, and have reworked this in #10248, so have added this to the `CoerceLogic` interface and implementations.
##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java
##########
@@ -685,6 +685,9 @@ IncrementalIndexRowResult toIncrementalIndexRow(InputRow row)
}
dimsKeySize += indexer.estimateEncodedKeyComponentSize(dimsKey);
// Set column capabilities as data is coming in
+ if (dimsKey == null) {
+ capabilities.setIsNullable(NullHandling.sqlCompatible());
Review comment:
This is now handled by the dimension indexers, which now can all determine if they have null values or not, so I don't think `NullHandling.isNullable(ValueType)` is necessary anymore.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469592335
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -335,6 +339,8 @@ private ColumnAnalysis analyzeComplexColumn(
{
try (final ComplexColumn complexColumn = columnHolder != null ? (ComplexColumn) columnHolder.getColumn() : null) {
final boolean hasMultipleValues = capabilities != null && capabilities.hasMultipleValues().isTrue();
+ // if we don't know for sure, then we should plan to check for nulls
+ final boolean nullable = capabilities != null && capabilities.hasNulls().isMaybeTrue();
Review comment:
Could you please rename this to `hasNulls`, for this reason: https://github.com/apache/druid/pull/10219#discussion_r469598220?
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -38,6 +38,7 @@
boolean hasSpatialIndexes();
Capable hasMultipleValues();
boolean isFilterable();
+ Capable isNullable();
Review comment:
I do feel that it shouldn't be called `isNullable`. I think we should avoid the word "nullable" in core Druid to refer to this concept, because one day, we might want to introduce the SQL-style concept of nullable vs non-nullable columns. That concept is more of a definition thing than a reality thing, and will be confusing to introduce to core if we have the reality-thing using the word "nullable".
Also, if we did introduce it to core, we'd probably want to report both (a column that is nullable table-wide might not actually have nulls in a specific segment, and that's useful to know).
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469536567
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -100,6 +100,11 @@ public long numRows(Segment segment)
Map<String, ColumnAnalysis> columns = new TreeMap<>();
+ Function<String, ColumnCapabilities> adapterCapabilitesFn =
Review comment:
eh, I think introduced the ugly `instanceof` in the first place in a previous PR to fix a different bug. I have a plan to not need this, since it is only necessary for incremental index adapter, so I hope this code is rather temporary but the changes are too big to be part of this patch.
I would rather not introduce a new method to storage adapter specifically for incremental index adapter, and would prefer to address it when I eliminate the need for it, if that is cool with you. If you disagree and feel strongly about it though, I can introduce a temporary method for it now, and just remove it in the follow-up.
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469592335
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -335,6 +339,8 @@ private ColumnAnalysis analyzeComplexColumn(
{
try (final ComplexColumn complexColumn = columnHolder != null ? (ComplexColumn) columnHolder.getColumn() : null) {
final boolean hasMultipleValues = capabilities != null && capabilities.hasMultipleValues().isTrue();
+ // if we don't know for sure, then we should plan to check for nulls
+ final boolean nullable = capabilities != null && capabilities.hasNulls().isMaybeTrue();
Review comment:
Could you please rename this to `hasNulls`?
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -192,6 +193,7 @@ private ColumnAnalysis analyzeNumericColumn(
return new ColumnAnalysis(
capabilities.getType().name(),
capabilities.hasMultipleValues().isTrue(),
+ capabilities.hasNulls().isMaybeTrue(), // if we don't know for sure, then we should plan to check for nulls
Review comment:
This comment doesn't make a lot of sense in the context of SegmentAnalyzer. It shouldn't "know" what the fields are being used for, so it shouldn't "know" they're going to be used to check for nulls somewhere higher up the chain.
Maybe a better thing to do would be:
1) Define "hasNulls" as meaning "definitely has nulls, or can't determine". This should be in the docs for the segmentMetadata query too.
2) Now this comment could read: "If we don't know for sure, return true to adhere to the definition of hasNulls"
##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -38,6 +38,7 @@
boolean hasSpatialIndexes();
Capable hasMultipleValues();
boolean isFilterable();
+ Capable isNullable();
Review comment:
I do feel strongly that it shouldn't be called `isNullable`. I think we should avoid the word "nullable" in core Druid to refer to this concept, because one day, we might want to introduce the SQL-style concept of nullable vs non-nullable columns. That concept is more of a definition thing than a reality thing, and will be confusing to introduce to core if we have the reality-thing using the word "nullable".
Also, if we did introduce it to core, we'd probably want to report both (a column that is nullable table-wide might not actually have nulls in a specific segment, and that's useful to know).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #10219: add hasNulls to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #10219:
URL: https://github.com/apache/druid/pull/10219
----------------------------------------------------------------
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
---------------------------------------------------------------------
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 change in pull request #10219: add isNullable to ColumnCapabilities, ColumnAnalysis
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10219:
URL: https://github.com/apache/druid/pull/10219#discussion_r469537834
##########
File path: processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java
##########
@@ -192,6 +193,7 @@ private ColumnAnalysis analyzeNumericColumn(
return new ColumnAnalysis(
capabilities.getType().name(),
capabilities.hasMultipleValues().isTrue(),
+ capabilities.isNullable().isTrue(),
Review comment:
I think maybe the only current unknowns will come from complex columns, which thinking about it should probably just report that they have nulls as true instead of unknown...
Right now nothing bad would happen if the segment metadata falsely reports not having nulls, but in the follow-up where this is wired into `DruidSchema`, falsely reporting nulls would cause the SQL planner to incorrectly plan to not handle nulls, so I think this should probably be modified to check `isMaybeTrue`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org