[format] Validate nested types for JSON tables - #9585
Open
LuciferYang wants to merge 2 commits into
Open
Conversation
JsonFileFormat.validateDataType let ARRAY, VECTOR, MAP and ROW fall through to one break, so it only ever looked at the outermost type root. A type outside the supported set nested inside one of them, say ARRAY<VARIANT>, passed create-table validation and then failed on the first write with a cast error from JsonFormatWriter, or on read with a bare NullPointerException. Recurse into the element, key, value and field types so the rejection happens up front with the message that was already written for it. This is what the other formats do: ORC through OrcTypeUtil.convertToOrcType, Parquet through ParquetSchemaConverter.convertToParquetType and Avro through AvroSchemaConverter.convertToSchema all walk into nested types and reject what they cannot represent. JSON was written like CSV, whose whitelist has no container types at all, but with the container types added to it. No table that works today stops working: the types this now rejects are exactly the ones that already fail at runtime, since none of MULTISET, VARIANT, BLOB, GEOMETRY or GEOGRAPHY has a cast rule to or from string. The comment claiming JSON supports all data types is removed with them; it was never true, the whitelist has always rejected the same set at the top level. Note this covers managed tables only. SchemaValidation is the only caller of validateDataFields, so a format table, which goes through CatalogUtils.validateCreateTable, is unaffected either way. Assisted-by: GLM-5.3
Contributor
|
+1 |
Contributor
|
Please rebase master. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
close #9584
JsonFileFormat.validateDataTypelet ARRAY, VECTOR, MAP and ROW fall through to onebreak, so it only ever looked at the outermost type root. A type outside the supported set nested inside one of them, sayARRAY<VARIANT>, passed create-table validation and then failed on the first write with a cast error fromJsonFormatWriter, or on read with a bareNullPointerExceptionwhereconvertPrimitiveStringToTypedereferences a null cast executor. This recurses into the element, key, value and field types, so the rejection happens up front with the message that was already written for it.That is what the other formats do: ORC through
OrcTypeUtil.convertToOrcType, Parquet throughParquetSchemaConverter.convertToParquetTypeand Avro throughAvroSchemaConverter.convertToSchemaall walk into nested types and reject what they cannot represent. JSON was written like CSV, whose whitelist has no container types at all, but with the container types added to it.No table that works today stops working. The types this now rejects are exactly the ones that already fail at runtime: none of
MULTISET,VARIANT,BLOB,GEOMETRYorGEOGRAPHYhas a cast rule to or from string, at any nesting depth. The comment claiming JSON supports all data types goes with them; it was never true, since the whitelist has always rejected that same set at the top level.Scope worth stating:
SchemaValidationis the only entry point forvalidateDataFields, so this covers managed tables. A format table is created throughCatalogUtils.validateCreateTable, which does not call it, and is unaffected either way.Tests
JsonFileFormatTest.testValidateRejectsUnsupportedNestedTypeasserts rejection at five nested positions:ARRAY<VARIANT>,MAP<VARIANT, STRING>,MAP<STRING, VARIANT>,ROW<INT, VARIANT>andARRAY<ROW<VARIANT>>. The two MAP shapes are separate because the key and the value are two independent recursion calls, and the last one covers two levels.It then validates the same shapes filled with supported types. That control is not decoration: it pins the
break;this change had to add to the primitive cases. Without it the primitives fall intocase ARRAYand die with aClassCastException, and all five rejection assertions would still pass in that state.Against the unfixed validator the first assertion fails with "Expecting code to raise a throwable".
mvn -pl paimon-format teston JDK 8: 597 tests, 0 failures.spotless:checkandcheckstyle:checkare clean.