kgyrtkirk commented on code in PR #15996:
URL: https://github.com/apache/druid/pull/15996#discussion_r1510750932
##########
processing/src/main/java/org/apache/druid/frame/read/columnar/DoubleFrameColumnReader.java:
##########
@@ -67,7 +67,7 @@ public ColumnPlus readColumn(final Frame frame)
return new ColumnPlus(
frameCol,
ColumnCapabilitiesImpl.createSimpleNumericColumnCapabilities(frameCol.getType())
- .setHasNulls(NullHandling.sqlCompatible() &&
frameCol.hasNulls),
Review Comment:
I think that:
* the system do allow `null` -s - via by a simple cast - so `null` exists
even in `defaultValue` mode - so I don't see it bad...
* I believe this codepath is only triggered for windowoperator related stuff
- but I'll check it (I'll update this comment when it finishes)
* the incoming frame declares that it has `null` values (which is being
covered)
* we also have the
[ClusteredGroupPartitionerTest#testDefaultClusteredGroupPartitionerWithNulls](https://github.com/apache/druid/blob/b3015bd7ced43e3510aff22dd6f8dc9f1d4781d0/processing/src/test/java/org/apache/druid/query/rowsandcols/semantic/ClusteredGroupPartitionerTest.java#L133)
which essentially inputs `null`-s as inputs...should I disable that testcase
for `defaultValues` ? it will most likely just hide the issue I was facing -
but I can live with that...should I just do that?
I believe the original logic incorrectly narrows to *doesn't have nulls*
(`NullHandling.sqlCompatible() && frameCol.hasNulls`) - which could be `false`
irrsepectively that the backing one does have (`frameCol.hasNulls`) ...if
that's not allowed an exception should be thrown right away and not take
further risks...
I see the following options:
* allow nulls - as the incoming frame already contained them
* raise an exception and mark the testcase
`ClusteredGroupPartitionerTest#testDefaultClusteredGroupPartitionerWithNulls`
not apply when `useDefaultValue` is enabled
--
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]
