From 83ae1facb891353ccbba1601be85ab5736421362 Mon Sep 17 00:00:00 2001 From: Hu Shenggang Date: Thu, 10 Sep 2026 17:56:24 +0800 Subject: [PATCH] [fix](nereids) Reject even argument count of json_set/json_insert/json_replace at analysis time ### What problem does this PR solve? Issue Number: None Problem Summary: `json_set`, `json_insert` and `json_replace` (and their `jsonb_*` aliases) take a JSON document followed by path/value pairs, so the argument count must be odd. The FE only checked `arity >= 3`, so a call whose last path had no value, e.g. `json_set('{}', '$.a', 1, '$.b')`, passed analysis and `EXPLAIN` succeeded. The error was only raised by the BE at execution time: ``` [INVALID_ARGUMENT]Function jsonb_set must have an odd number of arguments and more than 2 arguments, but got: 4 ``` The total argument count is known at analysis time, so this PR adds `checkLegalityBeforeTypeCoercion` to the three functions and rejects an even argument count with a clear `AnalysisException` before type coercion and the `to_json` rewrite. The BE check is kept as a defensive fallback. ### Release note None ### Check List (For Author) - Test: - Unit Test: `CheckExpressionLegalityTest#testJsonModifyFunctionsRejectEvenArity` - Regression test: `test_query_json_set`, `test_query_json_insert`, `test_query_json_replace` (4/6-argument rejection including `EXPLAIN` and the `jsonb_*` aliases, legal 3/5-argument calls) - Behavior changed: Yes (an even argument count is now rejected during FE analysis instead of failing at BE execution) - Does this need documentation: No Claude-Session: https://claude.ai/code/session_016LqgBtqMHamfbXB2sCDnuu --- .../functions/scalar/JsonInsert.java | 10 +++++++++ .../functions/scalar/JsonReplace.java | 10 +++++++++ .../expressions/functions/scalar/JsonSet.java | 10 +++++++++ .../analysis/CheckExpressionLegalityTest.java | 18 +++++++++++++++ .../json_function/test_query_json_insert.out | 3 +++ .../json_function/test_query_json_replace.out | 3 +++ .../json_function/test_query_json_set.out | 3 +++ .../test_query_json_insert.groovy | 22 +++++++++++++++++++ .../test_query_json_replace.groovy | 22 +++++++++++++++++++ .../json_function/test_query_json_set.groovy | 22 +++++++++++++++++++ 10 files changed, 123 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonInsert.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonInsert.java index a99df63983c2a2..06bedc8d522729 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonInsert.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonInsert.java @@ -18,6 +18,7 @@ package org.apache.doris.nereids.trees.expressions.functions.scalar; import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; @@ -64,6 +65,15 @@ public FunctionSignature customSignature() { return FunctionSignature.of(JsonType.INSTANCE, arguments); } + @Override + public void checkLegalityBeforeTypeCoercion() { + // arguments are a JSON document followed by (path, value) pairs, so arity must be odd + if ((arity() & 1) == 0) { + throw new AnalysisException(getName() + " requires a JSON document followed by path/value pairs," + + " so the number of arguments must be odd, but got " + arity() + ": " + this.toSql()); + } + } + /** * withChildren. */ diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonReplace.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonReplace.java index a4d06ddbaff6e7..c626d3ed5a8855 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonReplace.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonReplace.java @@ -18,6 +18,7 @@ package org.apache.doris.nereids.trees.expressions.functions.scalar; import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; @@ -64,6 +65,15 @@ public FunctionSignature customSignature() { return FunctionSignature.of(JsonType.INSTANCE, arguments); } + @Override + public void checkLegalityBeforeTypeCoercion() { + // arguments are a JSON document followed by (path, value) pairs, so arity must be odd + if ((arity() & 1) == 0) { + throw new AnalysisException(getName() + " requires a JSON document followed by path/value pairs," + + " so the number of arguments must be odd, but got " + arity() + ": " + this.toSql()); + } + } + /** * withChildren. */ diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonSet.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonSet.java index cdc9ed82b4f778..0f017d3d0d0ade 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonSet.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/JsonSet.java @@ -18,6 +18,7 @@ package org.apache.doris.nereids.trees.expressions.functions.scalar; import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.AlwaysNullable; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; @@ -64,6 +65,15 @@ public FunctionSignature customSignature() { return FunctionSignature.of(JsonType.INSTANCE, arguments); } + @Override + public void checkLegalityBeforeTypeCoercion() { + // arguments are a JSON document followed by (path, value) pairs, so arity must be odd + if ((arity() & 1) == 0) { + throw new AnalysisException(getName() + " requires a JSON document followed by path/value pairs," + + " so the number of arguments must be odd, but got " + arity() + ": " + this.toSql()); + } + } + /** * withChildren. */ diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckExpressionLegalityTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckExpressionLegalityTest.java index 447156afb38c95..21c11e6797e52d 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckExpressionLegalityTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckExpressionLegalityTest.java @@ -70,6 +70,24 @@ public void testArraySortLambdaArgumentCount() { }); } + @Test + public void testJsonModifyFunctionsRejectEvenArity() { + ConnectContext connectContext = MemoTestUtils.createConnectContext(); + for (String function : new String[] {"json_set", "jsonb_set", "json_insert", "jsonb_insert", + "json_replace", "jsonb_replace"}) { + ExceptionChecker.expectThrowsWithMsg(AnalysisException.class, + "number of arguments must be odd, but got 4", () -> + PlanChecker.from(connectContext) + .analyze("select " + function + "('{}', '$.a', 1, '$.b')")); + ExceptionChecker.expectThrowsWithMsg(AnalysisException.class, + "number of arguments must be odd, but got 6", () -> + PlanChecker.from(connectContext) + .analyze("select " + function + "('{}', '$.a', 1, '$.b', 2, '$.c')")); + PlanChecker.from(connectContext).analyze("select " + function + "('{}', '$.a', 1)"); + PlanChecker.from(connectContext).analyze("select " + function + "('{}', '$.a', 1, '$.b', 2)"); + } + } + @Test public void testCountDistinctBitmap() { ConnectContext connectContext = MemoTestUtils.createConnectContext(); diff --git a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_insert.out b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_insert.out index 9c6126266f91eb..e43d2018d7ae8e 100644 --- a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_insert.out +++ b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_insert.out @@ -93,3 +93,6 @@ -- !insert7 -- {"a":200} +-- !insert_odd_arity_ok -- +{"a":1} {"a":1,"b":2} {"a":1,"b":2} + diff --git a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_replace.out b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_replace.out index ac8cfb4501008f..3a67a270a042e8 100644 --- a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_replace.out +++ b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_replace.out @@ -80,3 +80,6 @@ null -- !replace8 -- {"a":100} +-- !replace_odd_arity_ok -- +{} {} {} + diff --git a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_set.out b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_set.out index b57a50e314bb5d..e198f86db0b5c4 100644 --- a/regression-test/data/query_p0/sql_functions/json_function/test_query_json_set.out +++ b/regression-test/data/query_p0/sql_functions/json_function/test_query_json_set.out @@ -85,3 +85,6 @@ null -- !set8 -- {"a":100} +-- !set_odd_arity_ok -- +{"a":1} {"a":1,"b":2} {"a":1,"b":2} + diff --git a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_insert.groovy b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_insert.groovy index 13893795fc1297..25d91e8c03c4f0 100644 --- a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_insert.groovy +++ b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_insert.groovy @@ -128,4 +128,26 @@ suite("test_query_json_insert", "query,arrow_flight_sql") { sql "select json_insert('1', '\$.', 4);" exception "Json path error: Invalid Json Path for value" } + + // arguments must be a JSON document followed by path/value pairs, so an even count is rejected by FE + test { + sql "select json_insert('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "select json_insert('{}', '\$.a', 1, '\$.b', 2, '\$.c');" + exception "number of arguments must be odd, but got 6" + } + + test { + sql "select jsonb_insert('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "explain select json_insert('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + qt_insert_odd_arity_ok """select json_insert('{}', '\$.a', 1), json_insert('{}', '\$.a', 1, '\$.b', 2), jsonb_insert('{}', '\$.a', 1, '\$.b', 2);""" } diff --git a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_replace.groovy b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_replace.groovy index 69f7142eff75b4..0b91ca39dbbddc 100644 --- a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_replace.groovy +++ b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_replace.groovy @@ -133,4 +133,26 @@ suite("test_query_json_replace", "query") { sql "select json_replace('1', '\$.', 4);" exception "Json path error: Invalid Json Path for value" } + + // arguments must be a JSON document followed by path/value pairs, so an even count is rejected by FE + test { + sql "select json_replace('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "select json_replace('{}', '\$.a', 1, '\$.b', 2, '\$.c');" + exception "number of arguments must be odd, but got 6" + } + + test { + sql "select jsonb_replace('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "explain select json_replace('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + qt_replace_odd_arity_ok """select json_replace('{}', '\$.a', 1), json_replace('{}', '\$.a', 1, '\$.b', 2), jsonb_replace('{}', '\$.a', 1, '\$.b', 2);""" } diff --git a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_set.groovy b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_set.groovy index 705e6046705e54..91174fa88271e9 100644 --- a/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_set.groovy +++ b/regression-test/suites/query_p0/sql_functions/json_function/test_query_json_set.groovy @@ -120,4 +120,26 @@ suite("test_query_json_set", "query") { sql "select json_set('1', '\$.', 4);" exception "Json path error: Invalid Json Path for value" } + + // arguments must be a JSON document followed by path/value pairs, so an even count is rejected by FE + test { + sql "select json_set('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "select json_set('{}', '\$.a', 1, '\$.b', 2, '\$.c');" + exception "number of arguments must be odd, but got 6" + } + + test { + sql "select jsonb_set('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + + test { + sql "explain select json_set('{}', '\$.a', 1, '\$.b');" + exception "number of arguments must be odd, but got 4" + } + qt_set_odd_arity_ok """select json_set('{}', '\$.a', 1), json_set('{}', '\$.a', 1, '\$.b', 2), jsonb_set('{}', '\$.a', 1, '\$.b', 2);""" }