From dad6ebc3e49bce8b14df7ecdf1e07750767e2def Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sun, 13 Sep 2026 06:19:18 +0800 Subject: [PATCH 1/3] [api] Auto-assign field id in DataTypeJsonParser.parseDataField MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public parseDataField(JsonNode) entry passed a null field counter, so parsing a field json without an "id" node threw a bare NullPointerException from the counter increment — while the sibling public parseDataType entry auto-assigns ids for exactly such input. Reachable from the create-function procedures via ParameterUtils.parseDataFieldArray on user-provided parameter json. Mirror parseDataType: pass a fresh counter so missing ids are auto-assigned and explicit ids keep working (the private overload's partial-id check is unchanged). Assisted-by: GLM-5.3 --- .../paimon/types/DataTypeJsonParser.java | 4 +- .../paimon/types/DataTypeJsonParserTest.java | 74 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java diff --git a/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java b/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java index 5076b577812b..9e1cbc7eaa15 100644 --- a/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java +++ b/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java @@ -39,7 +39,9 @@ public final class DataTypeJsonParser { public static DataField parseDataField(JsonNode json) { - return parseDataField(json, null); + // auto-assign the id when the json carries none, mirroring the public + // parseDataType entry; a null counter would NPE on such input + return parseDataField(json, new AtomicInteger(-1)); } private static DataField parseDataField(JsonNode json, AtomicInteger fieldId) { diff --git a/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java b/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java new file mode 100644 index 000000000000..f40677974e93 --- /dev/null +++ b/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.types; + +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.JsonNode; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.paimon.shade.jackson2.com.fasterxml.jackson.databind.node.ObjectNode; + +import org.junit.jupiter.api.Test; + +import java.util.Arrays; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Test for {@link DataTypeJsonParser}. */ +class DataTypeJsonParserTest { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + @Test + void parseDataFieldWithoutIdAutoAssigns() throws Exception { + ObjectNode json = MAPPER.createObjectNode(); + json.put("name", "x"); + json.put("type", "INT"); + + DataField field = DataTypeJsonParser.parseDataField(json); + assertThat(field.id()).isZero(); + assertThat(field.name()).isEqualTo("x"); + assertThat(field.type()).isEqualTo(new IntType()); + } + + @Test + void parseDataFieldKeepsExplicitId() throws Exception { + ObjectNode json = MAPPER.createObjectNode(); + json.put("id", 7); + json.put("name", "x"); + json.put("type", "INT"); + + DataField field = DataTypeJsonParser.parseDataField(json); + assertThat(field.id()).isEqualTo(7); + } + + @Test + void parseRowWithoutFieldIdsAutoAssignsSequentially() throws Exception { + JsonNode json = + MAPPER.readTree( + "{\"type\":\"ROW\",\"fields\":[{\"name\":\"a\",\"type\":\"INT\"}," + + "{\"name\":\"b\",\"type\":\"STRING\"}]}"); + + DataType type = DataTypeJsonParser.parseDataType(json); + assertThat(type) + .isEqualTo( + new RowType( + Arrays.asList( + new DataField(0, "a", new IntType()), + new DataField(1, "b", DataTypes.STRING())))); + } +} From 36a51be612e501bdfdffd017f67d38b1bc279254 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sun, 13 Sep 2026 12:42:12 +0800 Subject: [PATCH 2/3] fix: keep schema field ids strict, thread one counter for parameter lists The previous version made the public parseDataField(JsonNode) auto-assign, but that entry is also how SchemaSerializer.deserialize and the global Jackson DataField deserializer read table schemas, where a missing id would silently become 0 and table field ids drive projection and schema evolution. Keep that entry strict (now with a message instead of an NPE) and let the caller that genuinely has an id-less list, ParameterUtils.parseDataFieldArray, pass one counter for the whole array so sibling fields get 0, 1, 2 rather than all 0. Co-Authored-By: Claude Code --- .../paimon/types/DataTypeJsonParser.java | 12 ++++-- .../paimon/types/DataTypeJsonParserTest.java | 31 ++++++++++++--- .../apache/paimon/utils/ParameterUtils.java | 6 ++- .../paimon/utils/ParameterUtilsTest.java | 38 +++++++++++++++++++ 4 files changed, 76 insertions(+), 11 deletions(-) diff --git a/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java b/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java index 9e1cbc7eaa15..4ba89b8353d5 100644 --- a/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java +++ b/paimon-api/src/main/java/org/apache/paimon/types/DataTypeJsonParser.java @@ -39,18 +39,22 @@ public final class DataTypeJsonParser { public static DataField parseDataField(JsonNode json) { - // auto-assign the id when the json carries none, mirroring the public - // parseDataType entry; a null counter would NPE on such input - return parseDataField(json, new AtomicInteger(-1)); + return parseDataField(json, null); } - private static DataField parseDataField(JsonNode json, AtomicInteger fieldId) { + /** + * Parses a field, drawing its id from {@code fieldId} when the json carries none. Callers that + * parse a sequence of fields pass one counter for the whole sequence so the ids stay distinct; + * pass {@code null} to require an explicit id. + */ + public static DataField parseDataField(JsonNode json, AtomicInteger fieldId) { int id; JsonNode idNode = json.get("id"); if (idNode != null) { checkState(fieldId == null || fieldId.get() == -1, "Partial field id is not allowed."); id = idNode.asInt(); } else { + checkState(fieldId != null, "Field id is required but the field carries none."); id = fieldId.incrementAndGet(); } String name = json.get("name").asText(); diff --git a/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java b/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java index f40677974e93..18346049bf51 100644 --- a/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/types/DataTypeJsonParserTest.java @@ -25,8 +25,10 @@ import org.junit.jupiter.api.Test; import java.util.Arrays; +import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** Test for {@link DataTypeJsonParser}. */ class DataTypeJsonParserTest { @@ -34,19 +36,29 @@ class DataTypeJsonParserTest { private static final ObjectMapper MAPPER = new ObjectMapper(); @Test - void parseDataFieldWithoutIdAutoAssigns() throws Exception { + void parseDataFieldWithoutIdAndWithoutCounterIsRejected() { ObjectNode json = MAPPER.createObjectNode(); json.put("name", "x"); json.put("type", "INT"); - DataField field = DataTypeJsonParser.parseDataField(json); - assertThat(field.id()).isZero(); - assertThat(field.name()).isEqualTo("x"); - assertThat(field.type()).isEqualTo(new IntType()); + // a table schema must carry its field ids: they drive projection and schema evolution, + // so silently assigning one would be worse than refusing to parse + assertThatThrownBy(() -> DataTypeJsonParser.parseDataField(json)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Field id is required"); + } + + @Test + void parseDataFieldDrawsIdsFromOneCounter() { + AtomicInteger fieldId = new AtomicInteger(-1); + + assertThat(DataTypeJsonParser.parseDataField(fieldJson("a"), fieldId).id()).isZero(); + assertThat(DataTypeJsonParser.parseDataField(fieldJson("b"), fieldId).id()).isEqualTo(1); + assertThat(DataTypeJsonParser.parseDataField(fieldJson("c"), fieldId).id()).isEqualTo(2); } @Test - void parseDataFieldKeepsExplicitId() throws Exception { + void parseDataFieldKeepsExplicitId() { ObjectNode json = MAPPER.createObjectNode(); json.put("id", 7); json.put("name", "x"); @@ -71,4 +83,11 @@ void parseRowWithoutFieldIdsAutoAssignsSequentially() throws Exception { new DataField(0, "a", new IntType()), new DataField(1, "b", DataTypes.STRING())))); } + + private static ObjectNode fieldJson(String name) { + ObjectNode json = MAPPER.createObjectNode(); + json.put("name", name); + json.put("type", "INT"); + return json; + } } diff --git a/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java b/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java index e740940ffea5..be57539a344a 100644 --- a/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java +++ b/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java @@ -34,6 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -141,8 +142,11 @@ public static List parseDataFieldArray(String data) { if (data != null) { JsonNode jsonArray = JsonSerdeUtil.fromJson(data, JsonNode.class); if (jsonArray.isArray()) { + // one counter for the whole array: a user-supplied parameter list may omit the + // ids, and each field still needs its own + AtomicInteger fieldId = new AtomicInteger(-1); for (JsonNode objNode : jsonArray) { - DataField dataField = DataTypeJsonParser.parseDataField(objNode); + DataField dataField = DataTypeJsonParser.parseDataField(objNode, fieldId); list.add(dataField); } } diff --git a/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java b/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java index 47f1be885f50..9146ef715751 100644 --- a/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java @@ -18,9 +18,12 @@ package org.apache.paimon.utils; +import org.apache.paimon.types.DataField; + import org.junit.jupiter.api.Test; import java.util.Arrays; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -28,6 +31,41 @@ /** Tests for {@link ParameterUtils}. */ class ParameterUtilsTest { + @Test + void testParseDataFieldArrayWithoutIds() { + // create_function passes a user-written parameter list, which may omit the ids; each + // field still has to get its own instead of every one landing on 0 + List fields = + ParameterUtils.parseDataFieldArray( + "[{\"name\":\"a\",\"type\":\"INT\"}," + + "{\"name\":\"b\",\"type\":\"STRING\"}," + + "{\"name\":\"c\",\"type\":\"BIGINT\"}]"); + + assertThat(fields).extracting(DataField::id).containsExactly(0, 1, 2); + assertThat(fields).extracting(DataField::name).containsExactly("a", "b", "c"); + } + + @Test + void testParseDataFieldArrayKeepsExplicitIds() { + List fields = + ParameterUtils.parseDataFieldArray( + "[{\"id\":3,\"name\":\"a\",\"type\":\"INT\"}," + + "{\"id\":9,\"name\":\"b\",\"type\":\"STRING\"}]"); + + assertThat(fields).extracting(DataField::id).containsExactly(3, 9); + } + + @Test + void testParseDataFieldArrayRejectsPartialIds() { + assertThatThrownBy( + () -> + ParameterUtils.parseDataFieldArray( + "[{\"name\":\"a\",\"type\":\"INT\"}," + + "{\"id\":7,\"name\":\"b\",\"type\":\"STRING\"}]")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Partial field id is not allowed"); + } + @Test void testParseIntegerRanges() { assertThat(ParameterUtils.parseIntegerRanges("0-2, 4, 2, 6 - 7", 8)) From 3340566959058b26b7a703769bb704242d80edfb Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Sun, 13 Sep 2026 14:21:51 +0800 Subject: [PATCH 3/3] fix: reject a partially numbered parameter list in both orders The guard only fired when an id-less field came first: with the explicit id first the counter was still at -1, so the check passed and the id-less fields that followed drew 0 from it, colliding silently. That is the defect the change set out to remove. Decide up front instead: a list that carries any id at all gets no counter, so every field in it must carry one, and that also stops a nested id-less row inside a numbered list from drawing a colliding id. Co-Authored-By: Claude Code --- .../apache/paimon/utils/ParameterUtils.java | 17 +++++++++--- .../paimon/utils/ParameterUtilsTest.java | 27 ++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java b/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java index be57539a344a..b0308c90eade 100644 --- a/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java +++ b/paimon-common/src/main/java/org/apache/paimon/utils/ParameterUtils.java @@ -142,9 +142,11 @@ public static List parseDataFieldArray(String data) { if (data != null) { JsonNode jsonArray = JsonSerdeUtil.fromJson(data, JsonNode.class); if (jsonArray.isArray()) { - // one counter for the whole array: a user-supplied parameter list may omit the - // ids, and each field still needs its own - AtomicInteger fieldId = new AtomicInteger(-1); + // A counter only for a list that carries no ids at all, and one counter for the + // whole list so each field gets its own. Supplying it when some field already has + // an id would let the rest silently draw a colliding one, so in that case pass + // null and let the parser reject the list. + AtomicInteger fieldId = carriesAnyFieldId(jsonArray) ? null : new AtomicInteger(-1); for (JsonNode objNode : jsonArray) { DataField dataField = DataTypeJsonParser.parseDataField(objNode, fieldId); list.add(dataField); @@ -153,4 +155,13 @@ public static List parseDataFieldArray(String data) { } return list; } + + private static boolean carriesAnyFieldId(JsonNode jsonArray) { + for (JsonNode objNode : jsonArray) { + if (objNode.get("id") != null) { + return true; + } + } + return false; + } } diff --git a/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java b/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java index 9146ef715751..ded969871b04 100644 --- a/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/utils/ParameterUtilsTest.java @@ -57,13 +57,38 @@ void testParseDataFieldArrayKeepsExplicitIds() { @Test void testParseDataFieldArrayRejectsPartialIds() { + // both orders must be rejected: supplying a counter to a list that already carries an id + // would let the id-less fields silently draw a colliding one assertThatThrownBy( () -> ParameterUtils.parseDataFieldArray( "[{\"name\":\"a\",\"type\":\"INT\"}," + "{\"id\":7,\"name\":\"b\",\"type\":\"STRING\"}]")) .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("Partial field id is not allowed"); + .hasMessageContaining("Field id is required"); + + assertThatThrownBy( + () -> + ParameterUtils.parseDataFieldArray( + "[{\"id\":0,\"name\":\"a\",\"type\":\"INT\"}," + + "{\"name\":\"b\",\"type\":\"STRING\"}]")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Field id is required"); + } + + @Test + void testParseDataFieldArrayRejectsIdLessNestedField() { + // a nested row inside an explicitly numbered list would otherwise draw id 0 and collide + // with the first top-level field + assertThatThrownBy( + () -> + ParameterUtils.parseDataFieldArray( + "[{\"id\":0,\"name\":\"a\",\"type\":\"INT\"}," + + "{\"id\":1,\"name\":\"b\",\"type\":" + + "{\"type\":\"ROW\",\"fields\":" + + "[{\"name\":\"x\",\"type\":\"INT\"}]}}]")) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("Field id is required"); } @Test