Verify block types while generating JSON #27519
Conversation
core/trino-main/src/main/java/io/trino/operator/OutputSpoolingOperatorFactory.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/protocol/JsonEncodingUtils.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/protocol/JsonEncodingUtils.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/spooling-checks
branch
from
d33315b to
3c687f5
Compare
wendigo
force-pushed
the
serafin/spooling-checks
branch
from
3c687f5 to
22f5613
Compare
| } | ||
| } | ||
|
|
||
| private static String describeBlock(Block block) |
There was a problem hiding this comment.
Can we improve the Block#toString implementations to print this instead ?
It would be beneficial for debugging in general
| @Override | ||
| public boolean isSupported(Block block) | ||
| { | ||
| return block instanceof ByteArrayBlock; |
There was a problem hiding this comment.
| return block instanceof ByteArrayBlock; | |
| return block.getUnderlyingValueBlock() instanceof ByteArrayBlock; |
There was a problem hiding this comment.
at least according to actual requirements in encode
| case DictionaryBlock dictionaryBlock -> "DictionaryBlock{%s}".formatted(describeBlock(dictionaryBlock.getDictionary())); | ||
| case RunLengthEncodedBlock runLengthEncodedBlock -> "RleBlock{%s}".formatted(runLengthEncodedBlock.getValue()); |
There was a problem hiding this comment.
Handle via block.getUnderlyingValueBlock()
| case ArrayBlock arrayBlock -> "ArrayBlock{%s}".formatted(describeBlock(arrayBlock.getElementsBlock())); | ||
| case MapBlock mapBlock -> "MapBlock{keys=%s, values=%s}".formatted(describeBlock(mapBlock.getKeyBlock()), describeBlock(mapBlock.getValueBlock())); | ||
| case RowBlock rowBlock -> "RowBlock{fields=%s}".formatted(rowBlock.getFieldBlocks().stream().map(JsonEncodingUtils::describeBlock).collect(joining(", "))); | ||
| default -> block.toString(); |
There was a problem hiding this comment.
This code should not be here for ever.
Let's file an issue describing problem we're trying to identify and add a TODO comment + link here to eventually remove this boiler-plate.
Also, it would probably be enough to handle primitive types only. Code would be much simpler, while debuggability would be near exact the same.
| generator.flush(); // final flush to have the data written to the output stream | ||
| } | ||
| catch (Exception e) { | ||
| throwableConsumer.accept(new TrinoException(SERIALIZATION_ERROR, "Could not serialize data to JSON", e)); |
There was a problem hiding this comment.
This PR is quite a bit complex for a simple debugging log.
Did you consider maybe keeping track of deserialization state (change it as loop progresses) and add it to exception message here. This way you can easily get information like channel name or data types without all those code.
4 participants
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: