yashmayya commented on code in PR #13839:
URL: https://github.com/apache/pinot/pull/13839#discussion_r1721697298
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/mapper/SegmentMapper.java:
##########
@@ -95,7 +95,8 @@ public SegmentMapper(List<RecordReaderFileConfig>
recordReaderFileConfigs,
tableConfig.getIndexingConfig().getSortedColumn());
_fieldSpecs = pair.getLeft();
_numSortFields = pair.getRight();
- _includeNullFields =
tableConfig.getIndexingConfig().isNullHandlingEnabled();
+ _includeNullFields =
+ schema.isEnableColumnBasedNullHandling() ||
tableConfig.getIndexingConfig().isNullHandlingEnabled();
Review Comment:
> This means that when SegmentMapper is used with column based null handling
it will keep null values when writing rows. The main issue is that it will
treat all columns as nullable.
Does this mean that the null values for columns that are not nullable will
be handled appropriately in some later stage? Sorry if it's a dumb question,
I'm not too familiar with the segment processor framework.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -309,7 +309,11 @@ public void deleteSegmentFile() {
private String _stopReason = null;
private final Semaphore _segBuildSemaphore;
private final boolean _isOffHeap;
- private final boolean _nullHandlingEnabled;
+ /**
+ * Whether null handling is enabled by default. This value is only used if
+ * {@link Schema#isEnableColumnBasedNullHandling()} is false.
+ */
+ private final boolean _defaultNullHandlingEnabled;
Review Comment:
Should this be called `_tableLevelNullHandlingEnabled` instead to be even
more explicit?
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -309,7 +309,11 @@ public void deleteSegmentFile() {
private String _stopReason = null;
private final Semaphore _segBuildSemaphore;
private final boolean _isOffHeap;
- private final boolean _nullHandlingEnabled;
+ /**
+ * Whether null handling is enabled by default. This value is only used if
+ * {@link Schema#isEnableColumnBasedNullHandling()} is false.
+ */
+ private final boolean _defaultNullHandlingEnabled;
Review Comment:
Or is the intention here to make it clear that the "default" will be
overridden if column level null handling is enabled? I guess "table level"
could be misinterpreted as being higher priority than "column level" null
handling.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1002,7 +1002,8 @@ static void validatePartialUpsertStrategies(TableConfig
tableConfig, Schema sche
return;
}
-
Preconditions.checkState(tableConfig.getIndexingConfig().isNullHandlingEnabled(),
+ Preconditions.checkState(schema.isEnableColumnBasedNullHandling()
+ || tableConfig.getIndexingConfig().isNullHandlingEnabled(),
Review Comment:
A small unit test could be added in `TableConfigUtilsTest` for this.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1002,7 +1002,8 @@ static void validatePartialUpsertStrategies(TableConfig
tableConfig, Schema sche
return;
}
-
Preconditions.checkState(tableConfig.getIndexingConfig().isNullHandlingEnabled(),
+ Preconditions.checkState(schema.isEnableColumnBasedNullHandling()
+ || tableConfig.getIndexingConfig().isNullHandlingEnabled(),
Review Comment:
Also I'm not really familiar with upserts or partial upserts but do we not
need to make any checks on the columns that are nullable if column based null
handling is enabled 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
