kgyrtkirk commented on code in PR #15735:
URL: https://github.com/apache/druid/pull/15735#discussion_r1470791841
##########
processing/src/main/java/org/apache/druid/data/input/Rows.java:
##########
@@ -77,6 +79,8 @@ public static List<String> objectToStrings(final Object
inputValue)
} else if (inputValue instanceof byte[]) {
// convert byte[] to base64 encoded string
return Collections.singletonList(StringUtils.encodeBase64String((byte[])
inputValue));
+ } else if (inputValue instanceof ByteBuffer) {
+ return Collections.singletonList(StringUtils.fromUtf8(((ByteBuffer)
inputValue).array()));
Review Comment:
I agree that `byte[]` and `ByteBuffer` should be treated the same - using
the `encodeBase64String` for `ByteBuffer` makes things fail.
I find it odd to have such a conversion happen here in `objectsToString` - I
wonder if every caller expects this ...and where does the opposite side of the
conversion happens?
this conversion was introduced in [an ORC related
PR](https://github.com/apache/druid/pull/7138/files#diff-3cd12a73f4dea67083e7b369438f9359e70fa2be62fd7728878b54328972511dR69-R71)
- however the PR doesn't seem to contain base64 anywhere else; so I guess it
was just easy/convinient to add the conversion here.
I think possible options could be:
* do the base64 conversion in ORC related things
* I would prefer this - as I think a base64 conversions is unexpected here
* change the writer which have decided to add a `ByteBuffer`
The failing test which have induced these changes was
`RowsAndColumnsDecoratorTest`.
<details>
<summary>method invocation stacktrace</summary>
callsite to `objectToStrings` is
[here](https://github.com/apache/druid/blob/497e2123f04763174bfbf1992c8ae78406c952c6/processing/src/main/java/org/apache/druid/segment/RowBasedColumnSelectorFactory.java#L389)
```
java.lang.RuntimeException: objectToStringsByteA
at org.apache.druid.data.input.Rows.objectToStringsByteA(Rows.java:97)
at org.apache.druid.data.input.Rows.objectToStrings(Rows.java:86)
at
org.apache.druid.segment.RowBasedColumnSelectorFactory$2.updateCurrentValues(RowBasedColumnSelectorFactory.java:389)
at
org.apache.druid.segment.RowBasedColumnSelectorFactory$2.getRow(RowBasedColumnSelectorFactory.java:226)
at
org.apache.druid.frame.write.FrameWriterUtils.getUtf8ByteBuffersFromStringSelector(FrameWriterUtils.java:110)
at
org.apache.druid.frame.write.columnar.StringFrameColumnWriterImpl.getUtf8ByteBuffersFromSelector(StringFrameColumnWriter.java:309)
at
org.apache.druid.frame.write.columnar.StringFrameColumnWriterImpl.getUtf8ByteBuffersFromSelector(StringFrameColumnWriter.java:1)
at
org.apache.druid.frame.write.columnar.StringFrameColumnWriter.addSelection(StringFrameColumnWriter.java:98)
at
org.apache.druid.frame.write.columnar.ColumnarFrameWriter.addSelection(ColumnarFrameWriter.java:78)
at
org.apache.druid.query.rowsandcols.LazilyDecoratedRowsAndColumns.lambda$2(LazilyDecoratedRowsAndColumns.java:290)
at
org.apache.druid.java.util.common.guava.BaseSequence.accumulate(BaseSequence.java:44)
at
org.apache.druid.java.util.common.guava.WrappingSequence$1.get(WrappingSequence.java:50)
at
org.apache.druid.java.util.common.guava.SequenceWrapper.wrap(SequenceWrapper.java:55)
at
org.apache.druid.java.util.common.guava.WrappingSequence.accumulate(WrappingSequence.java:45)
at
org.apache.druid.query.rowsandcols.LazilyDecoratedRowsAndColumns.materializeStorageAdapter(LazilyDecoratedRowsAndColumns.java:235)
at
org.apache.druid.query.rowsandcols.LazilyDecoratedRowsAndColumns.materialize(LazilyDecoratedRowsAndColumns.java:192)
at
org.apache.druid.query.rowsandcols.LazilyDecoratedRowsAndColumns.maybeMaterialize(LazilyDecoratedRowsAndColumns.java:168)
at
org.apache.druid.query.rowsandcols.LazilyDecoratedRowsAndColumns.numRows(LazilyDecoratedRowsAndColumns.java:114)
at
org.apache.druid.query.rowsandcols.semantic.RowsAndColumnsDecoratorTest.validateDecorated(RowsAndColumnsDecoratorTest.java:225)
```
</details>
--
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]
