From ad93371a811f4977a0639986cf6f76c76f3a80c2 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Fri, 22 May 2026 10:38:22 -0700 Subject: [PATCH 01/14] Honor null handling in jsonExtractScalar SV transforms When query-level null handling is enabled and no default literal is set, the typed SV paths now emit SQL NULL for unresolved JSON paths instead of throwing IllegalArgumentException. --- .../JsonExtractScalarTransformFunction.java | 43 ++- ...sonExtractScalarTransformFunctionTest.java | 308 ++++++++++++++++++ 2 files changed, 347 insertions(+), 4 deletions(-) diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java index 94da2934f3..d324fb2d4b 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java @@ -41,6 +41,7 @@ import org.apache.pinot.core.util.NumericException; import org.apache.pinot.spi.data.FieldSpec.DataType; import org.apache.pinot.spi.utils.BooleanUtils; +import org.apache.pinot.spi.utils.CommonConstants.NullValuePlaceHolder; import org.apache.pinot.spi.utils.JsonUtils; import org.apache.pinot.spi.utils.TimestampUtils; import org.roaringbitmap.RoaringBitmap; @@ -55,9 +56,11 @@ /// - `jsonField` — single-value `STRING` or `BYTES` column / transform expression containing JSON. /// - `jsonPath` — JsonPath expression used to read the value. /// - `resultsType` — Pinot data type for the output. Append `_ARRAY` for multi-value results. -/// - `defaultValue` (optional) — used when the path resolves to `null` or fails. Without it, unresolved -/// SV rows throw `IllegalArgumentException`; MV rows surface as empty arrays, but null elements within -/// a resolved array still throw. +/// - `defaultValue` (optional) — used when the path resolves to `null` or fails. Without it, the +/// behavior depends on query-level null handling: when enabled, unresolved SV rows surface as SQL +/// `NULL` (typed zero/empty sentinel + a bit in [#getNullBitmap]); when disabled, unresolved SV +/// rows throw `IllegalArgumentException`. MV rows always surface as empty arrays when the path +/// doesn't resolve; null elements within a resolved array still throw if no default is set. /// /// **Supported `resultsType`:** `INT`, `LONG`, `FLOAT`, `DOUBLE`, `BIG_DECIMAL`, `BOOLEAN`, `TIMESTAMP`, /// `STRING`, `JSON`, `BYTES`, plus `_ARRAY` variants of `INT` / `LONG` / `FLOAT` / `DOUBLE` / @@ -98,6 +101,11 @@ public class JsonExtractScalarTransformFunction extends BaseTransformFunction { private DataType _storedType; private Object _defaultValue; private boolean _defaultIsNull; + // True when null-handling is enabled AND no default literal was supplied. In that mode, single-value + // transforms emit the type's zero/empty sentinel for unresolved JSON paths (instead of throwing) and + // surface the unresolved row through {@link #getNullBitmap}, matching the broker's null-handling + // contract for upstream operators (e.g. JsonIndexDistinctOperator). + private boolean _emitNullOnUnresolved; private TransformResultMetadata _resultMetadata; @Override @@ -178,6 +186,7 @@ public void init(List arguments, Map c ); } } + _emitNullOnUnresolved = _nullHandlingEnabled && _defaultValue == null; _resultMetadata = new TransformResultMetadata(_dataType, isSingleValue, false); } @@ -189,7 +198,7 @@ public TransformResultMetadata getResultMetadata() { @Override @Nullable public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { - if (!_defaultIsNull) { + if (!_defaultIsNull && !_emitNullOnUnresolved) { return super.getNullBitmap(valueBlock); } RoaringBitmap bitmap = new RoaringBitmap(); @@ -242,6 +251,12 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) { _intValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + // Write the null placeholder so stale data from a reused buffer can't surface. + // getNullBitmap marks the row null; consumers should read the bitmap, not this value. + _intValuesSV[i] = NullValuePlaceHolder.INT; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } @@ -271,6 +286,10 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) { _longValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + _longValuesSV[i] = NullValuePlaceHolder.LONG; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } @@ -299,6 +318,10 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) { _floatValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + _floatValuesSV[i] = NullValuePlaceHolder.FLOAT; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } @@ -327,6 +350,10 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) { _doubleValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + _doubleValuesSV[i] = NullValuePlaceHolder.DOUBLE; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } @@ -355,6 +382,10 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) { _bigDecimalValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + _bigDecimalValuesSV[i] = NullValuePlaceHolder.BIG_DECIMAL; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } @@ -383,6 +414,10 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { _stringValuesSV[i] = defaultValue; continue; } + if (_emitNullOnUnresolved) { + _stringValuesSV[i] = NullValuePlaceHolder.STRING; + continue; + } throw new IllegalArgumentException( "Cannot resolve JSON path on some records. Consider setting a default value."); } diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index 586880ae1f..c96b2361df 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -447,6 +447,314 @@ public void testIllegalArguments(String expressionStr) { TransformFunctionFactory.get(expression, _dataSourceMap); } + /** + * Country/click scenarios where the typed JSON path does NOT resolve to a value, spanning four + * JSON structural shapes: + *
    + *
  1. Flat: top-level scalar key missing, explicitly null, or empty doc.
    2. + *
  2. Nested object: {@code $.location.country} — inner key missing, parent JSON null, + * or root missing parent entirely.
    3. + *
  3. Array element: {@code $.tags[i]} — array null, empty, or index out-of-bounds.
    4. + *
  4. Array of objects: {@code $.events[0].country} — inner field null, array empty, + * array null, or root missing.
    5. + *
+ * JsonPath {@code SUPPRESS_EXCEPTIONS} folds every one of these into the same Java {@code null}, + * so all must produce SQL NULL when query-level null handling is enabled, and all must throw + * {@code IllegalArgumentException} when null handling is disabled and no default is set. + */ + @DataProvider(name = "countryClickUnresolvedCases") + public static Object[][] countryClickUnresolvedCases() { + return new Object[][]{ + // --- Flat / top-level scalar --- + {"STRING", "$.country", "{\"clicks\": 5}", "flat country missing"}, + {"STRING", "$.country", "{\"country\": null, \"clicks\": 5}", "flat country explicit-null"}, + {"STRING", "$.country", "{}", "flat empty doc"}, + // --- Flat / numeric across all typed SV paths --- + {"INT", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (INT)"}, + {"LONG", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (LONG)"}, + {"FLOAT", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (FLOAT)"}, + {"DOUBLE", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (DOUBLE)"}, + {"BIG_DECIMAL", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (BIG_DECIMAL)"}, + {"INT", "$.clicks", "{\"country\": \"US\", \"clicks\": null}", "flat clicks explicit-null (INT)"}, + // --- Nested object --- + {"STRING", "$.location.country", "{\"location\": {\"city\": \"SF\"}}", "nested inner key missing"}, + {"STRING", "$.location.country", "{\"location\": null}", "nested parent explicit-null"}, + {"STRING", "$.location.country", "{\"country\": \"US\"}", "nested parent missing"}, + // --- Array element --- + {"STRING", "$.tags[0]", "{\"tags\": null}", "array null parent"}, + {"STRING", "$.tags[0]", "{\"tags\": []}", "array empty"}, + {"STRING", "$.tags[1]", "{\"tags\": [\"red\"]}", "array index out-of-bounds"}, + {"STRING", "$.tags[0]", "{\"country\": \"US\"}", "array parent missing"}, + // --- Array of objects --- + {"STRING", "$.events[0].country", "{\"events\": [{\"country\": null}]}", "array-of-obj inner explicit-null"}, + {"STRING", "$.events[0].country", "{\"events\": [{}]}", "array-of-obj inner key missing"}, + {"STRING", "$.events[0].country", "{\"events\": []}", "array-of-obj empty array"}, + {"STRING", "$.events[0].country", "{\"events\": null}", "array-of-obj null array"}, + // --- Array of objects, numeric (INT) — confirms typed coverage at depth --- + {"INT", "$.events[0].clicks", "{\"events\": [{\"country\": \"US\"}]}", "array-of-obj inner numeric missing"} + }; + } + + /** + * E4 behavior: 3-arg {@code jsonExtractScalar} + query-level null handling ENABLED + unresolved + * path. The transform must surface SQL NULL in the projection instead of throwing, so the scalar + * fallback agrees with index-aware operators ({@code JsonIndexDistinctOperator}, + * {@code JsonIndexGroupByOperator}) on the null-group semantics. + */ + @Test(dataProvider = "countryClickUnresolvedCases") + public void testCountryClickNullHandlingOnEmitsNull(String resultsType, String jsonPath, String json, String label) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("testTable") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("json", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + Object[] expectedRow = new Object[]{null}; + FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(true) + .givenTable(schema, tableConfig) + .onFirstInstance(new Object[]{json}) + .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) + // TODO: Change the framework to do not duplicate segments when only one segment is used. + .thenResultIs(expectedRow, expectedRow); + } + + /** + * E3 behavior preservation: 3-arg {@code jsonExtractScalar} + query-level null handling DISABLED + * + unresolved path must still throw. The E4 fix is gated on {@code _nullHandlingEnabled}, so + * callers that haven't opted in see the legacy behavior. + */ + @Test(dataProvider = "countryClickUnresolvedCases") + public void testCountryClickNullHandlingOffThrows(String resultsType, String jsonPath, String json, String label) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("testTable") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("json", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + try { + FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(false) + .givenTable(schema, tableConfig) + .onFirstInstance(new Object[]{json}) + .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected query to fail for " + label); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()) + .describedAs("Case: " + label) + .contains("Cannot resolve JSON path on some records"); + } + } + + /** + * Positive cases: when the typed JSON path resolves — including at depth through nested objects, + * array elements, and array-of-objects — null-handling ON must surface the resolved value + * as-is. The fix only changes the unresolved-row behavior; resolved paths must be untouched at + * every nesting depth. + */ + @DataProvider(name = "countryClickResolvedCases") + public static Object[][] countryClickResolvedCases() { + return new Object[][]{ + // (resultsType, jsonPath, JSON doc, expected resolved value, label) + {"STRING", "$.country", "{\"country\": \"US\", \"clicks\": 5}", "US", "flat string"}, + {"INT", "$.clicks", "{\"country\": \"US\", \"clicks\": 5}", 5, "flat int"}, + {"STRING", "$.location.country", "{\"location\": {\"city\": \"SF\", \"country\": \"US\"}}", "US", + "nested string"}, + {"STRING", "$.tags[1]", "{\"tags\": [\"red\", \"blue\", \"green\"]}", "blue", "array element"}, + {"STRING", "$.events[0].country", "{\"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}", "US", + "array-of-obj inner"}, + {"INT", "$.events[1].clicks", "{\"events\": [{\"clicks\": 1}, {\"clicks\": 7}]}", 7, + "array-of-obj inner numeric"} + }; + } + + @Test(dataProvider = "countryClickResolvedCases") + public void testCountryClickNullHandlingOnResolvedValuePassThrough(String resultsType, String jsonPath, String json, + Object expectedValue, String label) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("testTable") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("json", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + Object[] expectedRow = new Object[]{expectedValue}; + FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(true) + .givenTable(schema, tableConfig) + .onFirstInstance(new Object[]{json}) + .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) + .thenResultIs(expectedRow, expectedRow); + } + + /** + * 4-arg with SQL NULL default + null-handling ON: pre-existing {@code _defaultIsNull} path — + * must continue producing SQL NULL at every nesting depth (regression guard so the E4 fix + * doesn't disturb this case). + */ + @DataProvider(name = "countryClickSqlNullDefaultCases") + public static Object[][] countryClickSqlNullDefaultCases() { + return new Object[][]{ + // (jsonPath, JSON document, label) + {"$.country", "{\"clicks\": 5}", "flat missing key"}, + {"$.location.country", "{\"location\": {\"city\": \"SF\"}}", "nested inner missing"}, + {"$.location.country", "{\"location\": null}", "nested parent null"}, + {"$.tags[0]", "{\"tags\": []}", "empty array"}, + {"$.tags[0]", "{\"tags\": null}", "null array"}, + {"$.events[0].country", "{\"events\": [{\"country\": null}]}", "array-of-obj inner explicit-null"}, + {"$.events[0].country", "{}", "deep path with empty root"} + }; + } + + @Test(dataProvider = "countryClickSqlNullDefaultCases") + public void testCountryClickFourArgSqlNullDefaultNullHandlingOn(String jsonPath, String json, String label) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("testTable") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("json", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + Object[] expectedRow = new Object[]{null}; + FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(true) + .givenTable(schema, tableConfig) + .onFirstInstance(new Object[]{json}) + // 4-arg form with SQL NULL literal as the default value; null-handling enabled. + .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', 'STRING', NULL) FROM testTable", jsonPath)) + .thenResultIs(expectedRow, expectedRow); + } + + // === Multi-row country-click fixture covering nested objects, arrays, and arrays of objects, + // used to exercise GROUP BY and DISTINCT through jsonExtractScalar with mixed resolution. === + + /// Four-row mixed-resolution country-click fixture. Across the rows, each path covered by the + /// GROUP BY / DISTINCT tests has at least one resolved value and at least one unresolved + /// occurrence (missing or explicit null), so the null group / null distinct value must show up. + private static final Object[][] COUNTRY_CLICK_FIXTURE = new Object[][]{ + // Row 0 — fully populated, two events. + {"{\"country\": \"US\", \"clicks\": 5, \"location\": {\"city\": \"SF\", \"country\": \"US\"}," + + " \"tags\": [\"red\", \"blue\", \"green\"], \"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"}, + // Row 1 — partial: location has no country; events[0].country is explicit null. + {"{\"country\": \"CA\", \"clicks\": 3, \"location\": {\"city\": \"Tor\"}," + + " \"tags\": [\"green\"], \"events\": [{\"country\": null}]}"}, + // Row 2 — every field explicitly JSON null. + {"{\"country\": null, \"clicks\": null, \"location\": null, \"tags\": null, \"events\": null}"}, + // Row 3 — empty doc. + {"{}"} + }; + + private FluentQueryTest.OnFirstInstance givenCountryClickFixture(boolean nullHandling) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("testTable") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("json", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("testTable") + .build(); + return FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(nullHandling) + .givenTable(schema, tableConfig) + .onFirstInstance(COUNTRY_CLICK_FIXTURE); + } + + /** + * GROUP BY across paths with mixed resolution. With null handling ON, the unresolved rows must + * collapse into a single null group (not throw, not split per row), and resolved values must + * group as usual. Segment duplication means the broker sees two copies of the fixture, so all + * counts are doubled — this is the framework convention, not a semantic claim about Pinot. + */ + @DataProvider(name = "countryClickGroupByCases") + public static Object[][] countryClickGroupByCases() { + // Path, list of expected (value, count) rows ordered by value ASC NULLS LAST. + // $.country : row0=US, row1=CA, row2=null, row3=null → US=1, CA=1, null=2 → ×2 segments + // $.location.country: row0=US, row1=missing, row2=null parent, row3=miss → US=1, null=3 + // $.tags[0] : row0=red, row1=green, row2=null array, row3=miss → red=1, green=1, null=2 + // $.events[0].country: row0=US, row1=null inner, row2=null array, row3=miss → US=1, null=3 + return new Object[][]{ + {"$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}}, "flat country"}, + {"$.location.country", new Object[][]{{"US", 2L}, {null, 6L}}, "nested country"}, + {"$.tags[0]", new Object[][]{{"green", 2L}, {"red", 2L}, {null, 4L}}, "array element"}, + {"$.events[0].country", new Object[][]{{"US", 2L}, {null, 6L}}, "array-of-obj inner country"} + }; + } + + @Test(dataProvider = "countryClickGroupByCases") + public void testCountryClickGroupByCountIncludesNullGroup(String jsonPath, Object[][] expectedRows, String label) { + String query = String.format( + "SELECT jsonExtractScalar(json, '%s', 'STRING') AS v, COUNT(*) FROM testTable " + + "GROUP BY v ORDER BY v ASC NULLS LAST", + jsonPath); + givenCountryClickFixture(true) + .whenQuery(query) + .thenResultIs(expectedRows); + } + + /** + * Symmetric check: with null handling OFF, GROUP BY across mixed-resolution rows must throw + * (preserves legacy behavior). The broker surfaces the failure as a query error which the + * FluentQueryTest framework escalates to an {@link AssertionError}. + */ + @Test + public void testCountryClickGroupByNullHandlingOffThrows() { + try { + givenCountryClickFixture(false) + .whenQuery("SELECT jsonExtractScalar(json, '$.country', 'STRING'), COUNT(*) FROM testTable GROUP BY 1") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected GROUP BY query to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); + } + } + + /** + * DISTINCT across paths with mixed resolution. With null handling ON, the distinct set must + * include exactly one null entry alongside the resolved values — null group must not blow up + * across segments (segment dedup), and must not throw. + */ + @DataProvider(name = "countryClickDistinctCases") + public static Object[][] countryClickDistinctCases() { + return new Object[][]{ + {"$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flat country"}, + {"$.location.country", new Object[][]{{"US"}, {null}}, "nested country"}, + {"$.tags[0]", new Object[][]{{"green"}, {"red"}, {null}}, "array element"}, + {"$.events[0].country", new Object[][]{{"US"}, {null}}, "array-of-obj inner country"} + }; + } + + @Test(dataProvider = "countryClickDistinctCases") + public void testCountryClickDistinctIncludesNull(String jsonPath, Object[][] expectedRows, String label) { + // Pinot requires the ORDER BY expression to match a DISTINCT column literally — positional + // ORDER BY (e.g. ORDER BY 1) is rejected with "ORDER-BY columns should be included in the + // DISTINCT columns". + String expr = String.format("jsonExtractScalar(json, '%s', 'STRING')", jsonPath); + String query = String.format( + "SELECT DISTINCT %s FROM testTable ORDER BY %s ASC NULLS LAST", expr, expr); + givenCountryClickFixture(true) + .whenQuery(query) + .thenResultIs(expectedRows); + } + + @Test + public void testCountryClickDistinctNullHandlingOffThrows() { + try { + givenCountryClickFixture(false) + .whenQuery("SELECT DISTINCT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected DISTINCT query to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); + } + } + @Test(dataProvider = "testParsingIllegalQueries", expectedExceptions = {SqlCompilationException.class}) public void testParsingIllegalQueries(String expressionStr) { RequestContextUtils.getExpression(expressionStr); From c424ff0183804871b44f517180403f4cee74b1c9 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Fri, 22 May 2026 11:48:49 -0700 Subject: [PATCH 02/14] Honor null handling in jsonExtractIndex SV transforms Mirror the scalar fix: when query-level null handling is enabled and no default literal is set, the six typed SV paths emit SQL NULL for unresolved JSON paths instead of throwing RuntimeException. --- .../JsonExtractIndexTransformFunction.java | 54 +++++++- ...JsonExtractIndexTransformFunctionTest.java | 116 ++++++++++++++++++ 2 files changed, 168 insertions(+), 2 deletions(-) diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java index 97b34e05e2..96970b11bb 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import javax.annotation.Nullable; import org.apache.pinot.common.function.JsonPathCache; import org.apache.pinot.core.operator.ColumnContext; import org.apache.pinot.core.operator.blocks.ValueBlock; @@ -32,6 +33,7 @@ import org.apache.pinot.segment.spi.index.IndexType; import org.apache.pinot.segment.spi.index.reader.JsonIndexReader; import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.apache.pinot.spi.utils.CommonConstants.NullValuePlaceHolder; import org.apache.pinot.spi.utils.JsonUtils; import org.roaringbitmap.RoaringBitmap; @@ -53,6 +55,11 @@ public class JsonExtractIndexTransformFunction extends BaseTransformFunction { private Map _valueToMatchingDocsMap; private boolean _isSingleValue; private String _filterJsonPath; + // True when null-handling is enabled AND no default literal was supplied for the SV path. In that + // mode, single-value transforms emit the type's null placeholder for unresolved JSON paths (instead + // of throwing) and surface the unresolved row through {@link #getNullBitmap}, matching the broker's + // null-handling contract for upstream operators such as {@code JsonIndexDistinctOperator}. + private boolean _emitNullOnUnresolved; @Override public String getName() { @@ -60,8 +67,9 @@ public String getName() { } @Override - public void init(List arguments, Map columnContextMap) { - super.init(arguments, columnContextMap); + public void init(List arguments, Map columnContextMap, + boolean nullHandlingEnabled) { + super.init(arguments, columnContextMap, nullHandlingEnabled); // Check that there are exactly 3 or 4 or 5 arguments if (arguments.size() < 3 || arguments.size() > 5) { throw new IllegalArgumentException( @@ -146,6 +154,7 @@ public void init(List arguments, Map c _filterJsonPath = ((LiteralTransformFunction) fifthArgument).getStringLiteral(); } + _emitNullOnUnresolved = _isSingleValue && _nullHandlingEnabled && _defaultValue == null; _resultMetadata = new TransformResultMetadata(dataType, _isSingleValue, false); } @@ -168,6 +177,10 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) { _intValuesSV[i] = (int) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _intValuesSV[i] = NullValuePlaceHolder.INT; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -190,6 +203,10 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) { _longValuesSV[i] = (long) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _longValuesSV[i] = NullValuePlaceHolder.LONG; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -212,6 +229,10 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) { _floatValuesSV[i] = (float) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _floatValuesSV[i] = NullValuePlaceHolder.FLOAT; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -234,6 +255,10 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) { _doubleValuesSV[i] = (double) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _doubleValuesSV[i] = NullValuePlaceHolder.DOUBLE; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -256,6 +281,10 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) { _bigDecimalValuesSV[i] = (BigDecimal) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _bigDecimalValuesSV[i] = NullValuePlaceHolder.BIG_DECIMAL; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -278,6 +307,10 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { _stringValuesSV[i] = (String) _defaultValue; continue; } + if (_emitNullOnUnresolved) { + _stringValuesSV[i] = NullValuePlaceHolder.STRING; + continue; + } throw new RuntimeException( String.format("Illegal Json Path: [%s], for docId [%s]", _jsonPathString, inputDocIds[i])); } @@ -286,6 +319,23 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { return _stringValuesSV; } + @Override + @Nullable + public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { + if (!_emitNullOnUnresolved) { + return super.getNullBitmap(valueBlock); + } + String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(), + getValueToMatchingDocsMap(), false); + RoaringBitmap nullBitmap = new RoaringBitmap(); + for (int i = 0; i < valuesFromIndex.length; i++) { + if (valuesFromIndex[i] == null) { + nullBitmap.add(i); + } + } + return nullBitmap.isEmpty() ? null : nullBitmap; + } + @Override public int[][] transformToIntValuesMV(ValueBlock valueBlock) { int numDocs = valueBlock.getNumDocs(); diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java index 7fcfb31b53..d494332e25 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java @@ -33,6 +33,8 @@ import org.apache.pinot.common.request.context.ExpressionContext; import org.apache.pinot.common.request.context.RequestContextUtils; import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.assertj.core.api.Assertions; +import org.roaringbitmap.RoaringBitmap; import org.testng.Assert; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -423,4 +425,118 @@ private String getValueForKey(String blob, JsonPath path) { private T getValueForKey(String blob, JsonPath path, TypeRef typeRef) { return JSON_PARSER_CONTEXT.parse(blob).read(path, typeRef); } + + // === Null-handling contract for unresolved JSON paths === + // + // The base fixture's JSON_STRING_SV_COLUMN has no '$.noField' key in any row, so the JSON-index + // lookup returns null for every doc. With query-level null handling ENABLED, the SV transforms + // must surface those rows as SQL NULL (type-default + a bit in getNullBitmap()), matching + // JsonIndexDistinctOperator's null-group semantics. With null handling DISABLED and no default + // literal, the legacy RuntimeException must be preserved. + + @DataProvider(name = "unresolvedPathSvTypes") + public Object[][] unresolvedPathSvTypes() { + return new Object[][]{ + {"INT", DataType.INT}, + {"LONG", DataType.LONG}, + {"FLOAT", DataType.FLOAT}, + {"DOUBLE", DataType.DOUBLE}, + {"BIG_DECIMAL", DataType.BIG_DECIMAL}, + {"STRING", DataType.STRING} + }; + } + + @Test(dataProvider = "unresolvedPathSvTypes") + public void testNullHandlingEnabledEmitsNullForUnresolvedPath(String resultsType, DataType resultsDataType) { + String expressionStr = String.format("jsonExtractIndex(%s,'$.noField','%s')", JSON_STRING_SV_COLUMN, resultsType); + ExpressionContext expression = RequestContextUtils.getExpression(expressionStr); + TransformFunction transformFunction = + TransformFunctionFactory.getNullHandlingEnabled(expression, _dataSourceMap); + + Assert.assertTrue(transformFunction instanceof JsonExtractIndexTransformFunction); + Assert.assertEquals(transformFunction.getResultMetadata().getDataType(), resultsDataType); + + switch (resultsDataType) { + case INT: + int[] intValues = transformFunction.transformToIntValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(intValues[i], 0); + } + break; + case LONG: + long[] longValues = transformFunction.transformToLongValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(longValues[i], 0L); + } + break; + case FLOAT: + float[] floatValues = transformFunction.transformToFloatValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(floatValues[i], 0f); + } + break; + case DOUBLE: + double[] doubleValues = transformFunction.transformToDoubleValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(doubleValues[i], 0d); + } + break; + case BIG_DECIMAL: + BigDecimal[] bigDecimalValues = transformFunction.transformToBigDecimalValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(bigDecimalValues[i].compareTo(BigDecimal.ZERO), 0); + } + break; + case STRING: + String[] stringValues = transformFunction.transformToStringValuesSV(_projectionBlock); + for (int i = 0; i < NUM_ROWS; i++) { + Assert.assertEquals(stringValues[i], ""); + } + break; + default: + throw new UnsupportedOperationException("Unsupported test type: " + resultsDataType); + } + + RoaringBitmap nullBitmap = transformFunction.getNullBitmap(_projectionBlock); + Assert.assertNotNull(nullBitmap, "Null bitmap must be populated when all rows are unresolved"); + Assert.assertEquals(nullBitmap.getCardinality(), NUM_ROWS, + "Every row should be marked null when the JSON path doesn't resolve"); + } + + @Test(dataProvider = "unresolvedPathSvTypes") + public void testNullHandlingDisabledStillThrowsForUnresolvedPath(String resultsType, DataType resultsDataType) { + String expressionStr = String.format("jsonExtractIndex(%s,'$.noField','%s')", JSON_STRING_SV_COLUMN, resultsType); + ExpressionContext expression = RequestContextUtils.getExpression(expressionStr); + TransformFunction transformFunction = TransformFunctionFactory.get(expression, _dataSourceMap); + + Assert.assertTrue(transformFunction instanceof JsonExtractIndexTransformFunction); + + try { + switch (resultsDataType) { + case INT: + transformFunction.transformToIntValuesSV(_projectionBlock); + break; + case LONG: + transformFunction.transformToLongValuesSV(_projectionBlock); + break; + case FLOAT: + transformFunction.transformToFloatValuesSV(_projectionBlock); + break; + case DOUBLE: + transformFunction.transformToDoubleValuesSV(_projectionBlock); + break; + case BIG_DECIMAL: + transformFunction.transformToBigDecimalValuesSV(_projectionBlock); + break; + case STRING: + transformFunction.transformToStringValuesSV(_projectionBlock); + break; + default: + throw new UnsupportedOperationException("Unsupported test type: " + resultsDataType); + } + Assert.fail("Expected RuntimeException when null handling is disabled"); + } catch (RuntimeException e) { + Assertions.assertThat(e.getMessage()).contains("Illegal Json Path"); + } + } } From 2610aef8c9d7fa4da6828e35cd46879f970948c6 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Fri, 22 May 2026 12:10:41 -0700 Subject: [PATCH 03/14] Refactor country/click tests for readability Named single-row JSON constants with pretty-printed comments, named fixture rows, helper methods that eliminate the inline FluentQueryTest boilerplate, and per-test Javadocs with a concrete example query and expected result. --- ...sonExtractScalarTransformFunctionTest.java | 553 +++++++++++------- 1 file changed, 336 insertions(+), 217 deletions(-) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index c96b2361df..56220a3fcb 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -447,100 +447,160 @@ public void testIllegalArguments(String expressionStr) { TransformFunctionFactory.get(expression, _dataSourceMap); } - /** - * Country/click scenarios where the typed JSON path does NOT resolve to a value, spanning four - * JSON structural shapes: - *
    - *
  1. Flat: top-level scalar key missing, explicitly null, or empty doc.
    2. - *
  2. Nested object: {@code $.location.country} — inner key missing, parent JSON null, - * or root missing parent entirely.
    3. - *
  3. Array element: {@code $.tags[i]} — array null, empty, or index out-of-bounds.
    4. - *
  4. Array of objects: {@code $.events[0].country} — inner field null, array empty, - * array null, or root missing.
    5. - *
- * JsonPath {@code SUPPRESS_EXCEPTIONS} folds every one of these into the same Java {@code null}, - * so all must produce SQL NULL when query-level null handling is enabled, and all must throw - * {@code IllegalArgumentException} when null handling is disabled and no default is set. - */ - @DataProvider(name = "countryClickUnresolvedCases") - public static Object[][] countryClickUnresolvedCases() { - return new Object[][]{ - // --- Flat / top-level scalar --- - {"STRING", "$.country", "{\"clicks\": 5}", "flat country missing"}, - {"STRING", "$.country", "{\"country\": null, \"clicks\": 5}", "flat country explicit-null"}, - {"STRING", "$.country", "{}", "flat empty doc"}, - // --- Flat / numeric across all typed SV paths --- - {"INT", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (INT)"}, - {"LONG", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (LONG)"}, - {"FLOAT", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (FLOAT)"}, - {"DOUBLE", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (DOUBLE)"}, - {"BIG_DECIMAL", "$.clicks", "{\"country\": \"US\"}", "flat clicks missing (BIG_DECIMAL)"}, - {"INT", "$.clicks", "{\"country\": \"US\", \"clicks\": null}", "flat clicks explicit-null (INT)"}, - // --- Nested object --- - {"STRING", "$.location.country", "{\"location\": {\"city\": \"SF\"}}", "nested inner key missing"}, - {"STRING", "$.location.country", "{\"location\": null}", "nested parent explicit-null"}, - {"STRING", "$.location.country", "{\"country\": \"US\"}", "nested parent missing"}, - // --- Array element --- - {"STRING", "$.tags[0]", "{\"tags\": null}", "array null parent"}, - {"STRING", "$.tags[0]", "{\"tags\": []}", "array empty"}, - {"STRING", "$.tags[1]", "{\"tags\": [\"red\"]}", "array index out-of-bounds"}, - {"STRING", "$.tags[0]", "{\"country\": \"US\"}", "array parent missing"}, - // --- Array of objects --- - {"STRING", "$.events[0].country", "{\"events\": [{\"country\": null}]}", "array-of-obj inner explicit-null"}, - {"STRING", "$.events[0].country", "{\"events\": [{}]}", "array-of-obj inner key missing"}, - {"STRING", "$.events[0].country", "{\"events\": []}", "array-of-obj empty array"}, - {"STRING", "$.events[0].country", "{\"events\": null}", "array-of-obj null array"}, - // --- Array of objects, numeric (INT) — confirms typed coverage at depth --- - {"INT", "$.events[0].clicks", "{\"events\": [{\"country\": \"US\"}]}", "array-of-obj inner numeric missing"} - }; - } + // ==================================================================================== + // === Country/click null-handling tests for jsonExtractScalar. === + // === === + // === The SV null-handling fix: with `enableNullHandling = true` and no default === + // === literal, an unresolved JSON path must surface as SQL NULL instead of throwing. === + // === These tests use a country/click data model — JSON documents shaped like === + // === `{country, clicks, location, tags, events}` — to exercise the fix across four === + // === JSON structural shapes (flat scalar, nested object, array element, array of === + // === objects) and three query shapes (projection, GROUP BY, DISTINCT). === + // ==================================================================================== + + // ---------------- Named single-row JSON documents (used by single-row tests) ---------------- + // + // Pretty-printed JSON for each constant is shown above its declaration. The string literal + // itself is kept compact — Jackson tolerates either form, but the comment above is what a + // human reads when scanning a test failure. + + // { "country": "US", "clicks": 5 } + private static final String CC_FLAT_RESOLVED = "{\"country\": \"US\", \"clicks\": 5}"; + // { "clicks": 5 } <- country key missing + private static final String CC_FLAT_COUNTRY_MISSING = "{\"clicks\": 5}"; + // { "country": null, "clicks": 5 } <- country explicit JSON null + private static final String CC_FLAT_COUNTRY_EXPLICIT_NULL = "{\"country\": null, \"clicks\": 5}"; + // { } <- empty document + private static final String CC_FLAT_EMPTY = "{}"; + // { "country": "US" } <- clicks key missing + private static final String CC_FLAT_CLICKS_MISSING = "{\"country\": \"US\"}"; + // { "country": "US", "clicks": null } <- clicks explicit JSON null + private static final String CC_FLAT_CLICKS_EXPLICIT_NULL = "{\"country\": \"US\", \"clicks\": null}"; + + // { "location": { "city": "SF", "country": "US" } } + private static final String CC_NESTED_RESOLVED = "{\"location\": {\"city\": \"SF\", \"country\": \"US\"}}"; + // { "location": { "city": "SF" } } <- nested inner key missing + private static final String CC_NESTED_INNER_MISSING = "{\"location\": {\"city\": \"SF\"}}"; + // { "location": null } <- nested parent explicit JSON null + private static final String CC_NESTED_PARENT_EXPLICIT_NULL = "{\"location\": null}"; + // { "country": "US" } <- nested parent itself missing + private static final String CC_NESTED_PARENT_MISSING = "{\"country\": \"US\"}"; + + // { "tags": ["red", "blue", "green"] } + private static final String CC_ARRAY_RESOLVED = "{\"tags\": [\"red\", \"blue\", \"green\"]}"; + // { "tags": null } <- array parent explicit JSON null + private static final String CC_ARRAY_NULL_PARENT = "{\"tags\": null}"; + // { "tags": [] } <- empty array + private static final String CC_ARRAY_EMPTY = "{\"tags\": []}"; + // { "tags": ["red"] } <- array index 1 out-of-bounds + private static final String CC_ARRAY_SINGLE_ELEM = "{\"tags\": [\"red\"]}"; + // { "country": "US" } <- array parent itself missing + private static final String CC_ARRAY_PARENT_MISSING = "{\"country\": \"US\"}"; + + // { "events": [{"country": "US"}, {"country": "CA"}] } + private static final String CC_AOO_RESOLVED = "{\"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"; + // { "events": [{"country": null}] } <- inner explicit JSON null + private static final String CC_AOO_INNER_EXPLICIT_NULL = "{\"events\": [{\"country\": null}]}"; + // { "events": [{}] } <- inner key missing + private static final String CC_AOO_INNER_KEY_MISSING = "{\"events\": [{}]}"; + // { "events": [] } <- empty array of objects + private static final String CC_AOO_EMPTY_ARRAY = "{\"events\": []}"; + // { "events": null } <- null array of objects + private static final String CC_AOO_NULL_ARRAY = "{\"events\": null}"; + // { "events": [{"country": "US"}] } <- inner numeric (clicks) missing + private static final String CC_AOO_INNER_NUMERIC_MISSING = "{\"events\": [{\"country\": \"US\"}]}"; + // { "events": [{"clicks": 1}, {"clicks": 7}] } + private static final String CC_AOO_NUMERIC_RESOLVED = "{\"events\": [{\"clicks\": 1}, {\"clicks\": 7}]}"; + + // ---------------- Multi-row fixture (used by GROUP BY / DISTINCT tests) ---------------- + // + // Pretty-printed JSON for each row: + // + // Row 0 — fully populated: + // { "country": "US", "clicks": 5, + // "location": { "city": "SF", "country": "US" }, + // "tags": ["red", "blue", "green"], + // "events": [{"country": "US"}, {"country": "CA"}] } + // + // Row 1 — partial: location has no country; events[0].country is explicit null: + // { "country": "CA", "clicks": 3, + // "location": { "city": "Tor" }, + // "tags": ["green"], + // "events": [{"country": null}] } + // + // Row 2 — every field explicit JSON null: + // { "country": null, "clicks": null, "location": null, "tags": null, "events": null } + // + // Row 3 — empty document: + // {} + // + // Across the four rows, every path covered by the GROUP BY / DISTINCT tests has at least one + // resolved value AND at least one unresolved occurrence (missing key or explicit null), so + // both the value groups and the null group must surface in the result. + + private static final String CC_ROW_0_FULL = "{\"country\": \"US\", \"clicks\": 5," + + " \"location\": {\"city\": \"SF\", \"country\": \"US\"}," + + " \"tags\": [\"red\", \"blue\", \"green\"]," + + " \"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"; + + private static final String CC_ROW_1_PARTIAL = "{\"country\": \"CA\", \"clicks\": 3," + + " \"location\": {\"city\": \"Tor\"}," + + " \"tags\": [\"green\"]," + + " \"events\": [{\"country\": null}]}"; + + private static final String CC_ROW_2_ALL_NULL = + "{\"country\": null, \"clicks\": null, \"location\": null, \"tags\": null, \"events\": null}"; + + private static final String CC_ROW_3_EMPTY = "{}"; + + private static final Object[][] COUNTRY_CLICK_FIXTURE = { + {CC_ROW_0_FULL}, + {CC_ROW_1_PARTIAL}, + {CC_ROW_2_ALL_NULL}, + {CC_ROW_3_EMPTY} + }; - /** - * E4 behavior: 3-arg {@code jsonExtractScalar} + query-level null handling ENABLED + unresolved - * path. The transform must surface SQL NULL in the projection instead of throwing, so the scalar - * fallback agrees with index-aware operators ({@code JsonIndexDistinctOperator}, - * {@code JsonIndexGroupByOperator}) on the null-group semantics. - */ - @Test(dataProvider = "countryClickUnresolvedCases") - public void testCountryClickNullHandlingOnEmitsNull(String resultsType, String jsonPath, String json, String label) { + // ---------------- Helpers ---------------- + + private FluentQueryTest.OnFirstInstance givenSingleRowJsonTable(boolean nullHandling, String json) { Schema schema = new Schema.SchemaBuilder() .setSchemaName("testTable") .setEnableColumnBasedNullHandling(true) .addDimensionField("json", DataType.JSON) .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("testTable") - .build(); - Object[] expectedRow = new Object[]{null}; - FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(true) + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + return FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(nullHandling) .givenTable(schema, tableConfig) - .onFirstInstance(new Object[]{json}) - .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) - // TODO: Change the framework to do not duplicate segments when only one segment is used. - .thenResultIs(expectedRow, expectedRow); + .onFirstInstance(new Object[]{json}); } - /** - * E3 behavior preservation: 3-arg {@code jsonExtractScalar} + query-level null handling DISABLED - * + unresolved path must still throw. The E4 fix is gated on {@code _nullHandlingEnabled}, so - * callers that haven't opted in see the legacy behavior. - */ - @Test(dataProvider = "countryClickUnresolvedCases") - public void testCountryClickNullHandlingOffThrows(String resultsType, String jsonPath, String json, String label) { + private FluentQueryTest.OnFirstInstance givenCountryClickFixture(boolean nullHandling) { Schema schema = new Schema.SchemaBuilder() .setSchemaName("testTable") .setEnableColumnBasedNullHandling(true) .addDimensionField("json", DataType.JSON) .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("testTable") - .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); + return FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(nullHandling) + .givenTable(schema, tableConfig) + .onFirstInstance(COUNTRY_CLICK_FIXTURE); + } + + /** Asserts that the 3-arg `jsonExtractScalar(...)` query returns SQL NULL for a single-row fixture. */ + private void assertSingleRowReturnsNull(boolean nullHandling, String json, String jsonPath, String resultsType) { + Object[] expectedNullRow = new Object[]{null}; + givenSingleRowJsonTable(nullHandling, json) + .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) + // Segment duplication: the same row is returned twice (one per duplicated segment). + .thenResultIs(expectedNullRow, expectedNullRow); + } + + /** Asserts that the 3-arg `jsonExtractScalar(...)` query throws when null handling is off. */ + private void assertSingleRowThrowsForUnresolved(String json, String jsonPath, String resultsType, String label) { try { - FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(false) - .givenTable(schema, tableConfig) - .onFirstInstance(new Object[]{json}) + givenSingleRowJsonTable(false, json) .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) .thenResultIs(new Object[]{"unused"}); Assert.fail("Expected query to fail for " + label); @@ -551,160 +611,211 @@ public void testCountryClickNullHandlingOffThrows(String resultsType, String jso } } + // ---------------- Unresolved paths: null-handling ON must surface SQL NULL ---------------- + /** - * Positive cases: when the typed JSON path resolves — including at depth through nested objects, - * array elements, and array-of-objects — null-handling ON must surface the resolved value - * as-is. The fix only changes the unresolved-row behavior; resolved paths must be untouched at - * every nesting depth. + * The SV null-handling fix. With null handling on and no default literal, an unresolved JSON + * path must surface as SQL NULL in the projection — matching the contract that + * {@code JsonIndexDistinctOperator} already provides for the DISTINCT route. + *

+ * Example (flat, missing key): + * 

+   *   JSON:   { "clicks": 5 }
+   *   Query:  SET enableNullHandling = true;
+   *           SELECT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
+   *   Result: NULL
+   *
+ * See {@link #unresolvedSinglePathCases()} for the full per-case matrix. */ - @DataProvider(name = "countryClickResolvedCases") - public static Object[][] countryClickResolvedCases() { + @Test(dataProvider = "unresolvedSinglePathCases") + public void testNullHandlingOnReturnsNullForUnresolvedPath(String resultsType, String jsonPath, String json, + String label) { + assertSingleRowReturnsNull(true, json, jsonPath, resultsType); + } + + /** + * Legacy behavior preservation. With null handling off and no default, the same unresolved-path + * cases must throw — the E4 fix is gated on {@code _nullHandlingEnabled}, so callers that + * haven't opted in see no behavior change. + *

+ * Example (flat, missing key): + * 

+   *   JSON:   { "clicks": 5 }
+   *   Query:  SELECT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
+   *   Result: throws IllegalArgumentException("Cannot resolve JSON path on some records.…")
+   *
+ */ + @Test(dataProvider = "unresolvedSinglePathCases") + public void testNullHandlingOffThrowsForUnresolvedPath(String resultsType, String jsonPath, String json, + String label) { + assertSingleRowThrowsForUnresolved(json, jsonPath, resultsType, label); + } + + /** + * Each row is `(resultsType, jsonPath, JSON document, structural-shape label)`. Every case is + * one where `jsonPath` does NOT resolve in the given JSON document — that's what makes it + * exercise the null-handling code path. + */ + @DataProvider(name = "unresolvedSinglePathCases") + public static Object[][] unresolvedSinglePathCases() { return new Object[][]{ - // (resultsType, jsonPath, JSON doc, expected resolved value, label) - {"STRING", "$.country", "{\"country\": \"US\", \"clicks\": 5}", "US", "flat string"}, - {"INT", "$.clicks", "{\"country\": \"US\", \"clicks\": 5}", 5, "flat int"}, - {"STRING", "$.location.country", "{\"location\": {\"city\": \"SF\", \"country\": \"US\"}}", "US", - "nested string"}, - {"STRING", "$.tags[1]", "{\"tags\": [\"red\", \"blue\", \"green\"]}", "blue", "array element"}, - {"STRING", "$.events[0].country", "{\"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}", "US", - "array-of-obj inner"}, - {"INT", "$.events[1].clicks", "{\"events\": [{\"clicks\": 1}, {\"clicks\": 7}]}", 7, - "array-of-obj inner numeric"} + // FLAT — string path + {"STRING", "$.country", CC_FLAT_COUNTRY_MISSING, "flat: country key missing"}, + {"STRING", "$.country", CC_FLAT_COUNTRY_EXPLICIT_NULL, "flat: country explicit JSON null"}, + {"STRING", "$.country", CC_FLAT_EMPTY, "flat: empty document"}, + // FLAT — numeric path, all 5 numeric SV types + {"INT", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (INT)"}, + {"LONG", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (LONG)"}, + {"FLOAT", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (FLOAT)"}, + {"DOUBLE", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (DOUBLE)"}, + {"BIG_DECIMAL", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (BIG_DECIMAL)"}, + {"INT", "$.clicks", CC_FLAT_CLICKS_EXPLICIT_NULL, "flat: clicks explicit JSON null (INT)"}, + // NESTED OBJECT + {"STRING", "$.location.country", CC_NESTED_INNER_MISSING, "nested: inner key missing"}, + {"STRING", "$.location.country", CC_NESTED_PARENT_EXPLICIT_NULL, "nested: parent explicit JSON null"}, + {"STRING", "$.location.country", CC_NESTED_PARENT_MISSING, "nested: parent key missing"}, + // ARRAY ELEMENT + {"STRING", "$.tags[0]", CC_ARRAY_NULL_PARENT, "array: parent explicit JSON null"}, + {"STRING", "$.tags[0]", CC_ARRAY_EMPTY, "array: empty array"}, + {"STRING", "$.tags[1]", CC_ARRAY_SINGLE_ELEM, "array: index out-of-bounds"}, + {"STRING", "$.tags[0]", CC_ARRAY_PARENT_MISSING, "array: parent key missing"}, + // ARRAY OF OBJECTS + {"STRING", "$.events[0].country", CC_AOO_INNER_EXPLICIT_NULL, "array-of-obj: inner explicit JSON null"}, + {"STRING", "$.events[0].country", CC_AOO_INNER_KEY_MISSING, "array-of-obj: inner key missing"}, + {"STRING", "$.events[0].country", CC_AOO_EMPTY_ARRAY, "array-of-obj: empty array"}, + {"STRING", "$.events[0].country", CC_AOO_NULL_ARRAY, "array-of-obj: null array"}, + // ARRAY OF OBJECTS — numeric (INT) confirms typed coverage at depth + {"INT", "$.events[0].clicks", CC_AOO_INNER_NUMERIC_MISSING, "array-of-obj: inner numeric missing (INT)"} }; } - @Test(dataProvider = "countryClickResolvedCases") - public void testCountryClickNullHandlingOnResolvedValuePassThrough(String resultsType, String jsonPath, String json, + // ---------------- Resolved paths: null-handling ON must pass values through ---------------- + + /** + * Resolved-path regression guard. The fix only changes the unresolved-row behavior; resolved + * paths must still produce their values verbatim at every JSON nesting depth. + *

+ * Example (array of objects, numeric): + * 

+   *   JSON:   { "events": [{"clicks": 1}, {"clicks": 7}] }
+   *   Query:  SET enableNullHandling = true;
+   *           SELECT jsonExtractScalar(json, '$.events[1].clicks', 'INT') FROM testTable
+   *   Result: 7
+   *
+ */ + @Test(dataProvider = "resolvedSinglePathCases") + public void testNullHandlingOnPassesResolvedValueThrough(String resultsType, String jsonPath, String json, Object expectedValue, String label) { - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("testTable") - .setEnableColumnBasedNullHandling(true) - .addDimensionField("json", DataType.JSON) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("testTable") - .build(); Object[] expectedRow = new Object[]{expectedValue}; - FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(true) - .givenTable(schema, tableConfig) - .onFirstInstance(new Object[]{json}) + givenSingleRowJsonTable(true, json) .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) .thenResultIs(expectedRow, expectedRow); } - /** - * 4-arg with SQL NULL default + null-handling ON: pre-existing {@code _defaultIsNull} path — - * must continue producing SQL NULL at every nesting depth (regression guard so the E4 fix - * doesn't disturb this case). - */ - @DataProvider(name = "countryClickSqlNullDefaultCases") - public static Object[][] countryClickSqlNullDefaultCases() { + /** Each row is `(resultsType, jsonPath, JSON document, expected resolved value, label)`. */ + @DataProvider(name = "resolvedSinglePathCases") + public static Object[][] resolvedSinglePathCases() { return new Object[][]{ - // (jsonPath, JSON document, label) - {"$.country", "{\"clicks\": 5}", "flat missing key"}, - {"$.location.country", "{\"location\": {\"city\": \"SF\"}}", "nested inner missing"}, - {"$.location.country", "{\"location\": null}", "nested parent null"}, - {"$.tags[0]", "{\"tags\": []}", "empty array"}, - {"$.tags[0]", "{\"tags\": null}", "null array"}, - {"$.events[0].country", "{\"events\": [{\"country\": null}]}", "array-of-obj inner explicit-null"}, - {"$.events[0].country", "{}", "deep path with empty root"} + {"STRING", "$.country", CC_FLAT_RESOLVED, "US", "flat string"}, + {"INT", "$.clicks", CC_FLAT_RESOLVED, 5, "flat int"}, + {"STRING", "$.location.country", CC_NESTED_RESOLVED, "US", "nested string"}, + {"STRING", "$.tags[1]", CC_ARRAY_RESOLVED, "blue", "array element"}, + {"STRING", "$.events[0].country", CC_AOO_RESOLVED, "US", "array-of-obj inner string"}, + {"INT", "$.events[1].clicks", CC_AOO_NUMERIC_RESOLVED, 7, "array-of-obj inner numeric"} }; } - @Test(dataProvider = "countryClickSqlNullDefaultCases") - public void testCountryClickFourArgSqlNullDefaultNullHandlingOn(String jsonPath, String json, String label) { - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("testTable") - .setEnableColumnBasedNullHandling(true) - .addDimensionField("json", DataType.JSON) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("testTable") - .build(); + // ---------------- 4-arg with SQL NULL default + null-handling ON (regression guard) ---------------- + + /** + * 4-arg `jsonExtractScalar(json, path, type, NULL)` + null handling ON. This is the pre-existing + * {@code _defaultIsNull} code path — must continue producing SQL NULL at every nesting depth so + * the E4 fix doesn't disturb it. + *

+ * Example (nested, inner missing): + * 

+   *   JSON:   { "location": { "city": "SF" } }
+   *   Query:  SET enableNullHandling = true;
+   *           SELECT jsonExtractScalar(json, '$.location.country', 'STRING', NULL) FROM testTable
+   *   Result: NULL
+   *
+ */ + @Test(dataProvider = "fourArgSqlNullDefaultCases") + public void testFourArgSqlNullDefaultReturnsNullWithNullHandlingOn(String jsonPath, String json, String label) { Object[] expectedRow = new Object[]{null}; - FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(true) - .givenTable(schema, tableConfig) - .onFirstInstance(new Object[]{json}) - // 4-arg form with SQL NULL literal as the default value; null-handling enabled. + givenSingleRowJsonTable(true, json) .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', 'STRING', NULL) FROM testTable", jsonPath)) .thenResultIs(expectedRow, expectedRow); } - // === Multi-row country-click fixture covering nested objects, arrays, and arrays of objects, - // used to exercise GROUP BY and DISTINCT through jsonExtractScalar with mixed resolution. === - - /// Four-row mixed-resolution country-click fixture. Across the rows, each path covered by the - /// GROUP BY / DISTINCT tests has at least one resolved value and at least one unresolved - /// occurrence (missing or explicit null), so the null group / null distinct value must show up. - private static final Object[][] COUNTRY_CLICK_FIXTURE = new Object[][]{ - // Row 0 — fully populated, two events. - {"{\"country\": \"US\", \"clicks\": 5, \"location\": {\"city\": \"SF\", \"country\": \"US\"}," - + " \"tags\": [\"red\", \"blue\", \"green\"], \"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"}, - // Row 1 — partial: location has no country; events[0].country is explicit null. - {"{\"country\": \"CA\", \"clicks\": 3, \"location\": {\"city\": \"Tor\"}," - + " \"tags\": [\"green\"], \"events\": [{\"country\": null}]}"}, - // Row 2 — every field explicitly JSON null. - {"{\"country\": null, \"clicks\": null, \"location\": null, \"tags\": null, \"events\": null}"}, - // Row 3 — empty doc. - {"{}"} - }; + /** Each row is `(jsonPath, JSON document, label)` — all rows pick a path that does NOT resolve. */ + @DataProvider(name = "fourArgSqlNullDefaultCases") + public static Object[][] fourArgSqlNullDefaultCases() { + return new Object[][]{ + {"$.country", CC_FLAT_COUNTRY_MISSING, "flat: key missing"}, + {"$.location.country", CC_NESTED_INNER_MISSING, "nested: inner missing"}, + {"$.location.country", CC_NESTED_PARENT_EXPLICIT_NULL, "nested: parent explicit JSON null"}, + {"$.tags[0]", CC_ARRAY_EMPTY, "array: empty"}, + {"$.tags[0]", CC_ARRAY_NULL_PARENT, "array: null parent"}, + {"$.events[0].country", CC_AOO_INNER_EXPLICIT_NULL, "array-of-obj: inner explicit JSON null"}, + {"$.events[0].country", CC_FLAT_EMPTY, "deep path against empty root"} + }; + } - private FluentQueryTest.OnFirstInstance givenCountryClickFixture(boolean nullHandling) { - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("testTable") - .setEnableColumnBasedNullHandling(true) - .addDimensionField("json", DataType.JSON) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) - .setTableName("testTable") - .build(); - return FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(nullHandling) - .givenTable(schema, tableConfig) - .onFirstInstance(COUNTRY_CLICK_FIXTURE); + // ---------------- GROUP BY: null-handling ON must produce a single null group ---------------- + + /** + * GROUP BY over the multi-row country/click fixture, with null handling ON. The unresolved rows + * (per the row breakdown in the fixture Javadoc above) must collapse into a single null group + * with the correct count, alongside the resolved value groups. + *

+ * Example (flat country): + * 

+   *   Rows (×2 for segment duplication):
+   *     Row 0: country = "US"        Row 1: country = "CA"
+   *     Row 2: country = JSON null   Row 3: country missing
+   *
+   *   Query: SELECT jsonExtractScalar(json, '$.country', 'STRING') AS v, COUNT(*)
+   *          FROM testTable GROUP BY v ORDER BY v ASC NULLS LAST
+   *
+   *   Result rows: ("CA", 2), ("US", 2), (NULL, 4)
+   *
+ * The data provider lists the expected (value, count) rows for each path covered. + */ + @Test(dataProvider = "groupByCases") + public void testGroupByOnUnresolvedPathProducesNullGroup(String jsonPath, Object[][] expectedRows, String label) { + String query = String.format( + "SELECT jsonExtractScalar(json, '%s', 'STRING') AS v, COUNT(*) FROM testTable " + + "GROUP BY v ORDER BY v ASC NULLS LAST", + jsonPath); + givenCountryClickFixture(true).whenQuery(query).thenResultIs(expectedRows); } /** - * GROUP BY across paths with mixed resolution. With null handling ON, the unresolved rows must - * collapse into a single null group (not throw, not split per row), and resolved values must - * group as usual. Segment duplication means the broker sees two copies of the fixture, so all - * counts are doubled — this is the framework convention, not a semantic claim about Pinot. + * Each row is `(jsonPath, expected (value, count) rows after ORDER BY v ASC NULLS LAST, label)`. + * Counts are ×2 the per-row count due to segment duplication (one segment becomes two). */ - @DataProvider(name = "countryClickGroupByCases") - public static Object[][] countryClickGroupByCases() { - // Path, list of expected (value, count) rows ordered by value ASC NULLS LAST. - // $.country : row0=US, row1=CA, row2=null, row3=null → US=1, CA=1, null=2 → ×2 segments - // $.location.country: row0=US, row1=missing, row2=null parent, row3=miss → US=1, null=3 - // $.tags[0] : row0=red, row1=green, row2=null array, row3=miss → red=1, green=1, null=2 - // $.events[0].country: row0=US, row1=null inner, row2=null array, row3=miss → US=1, null=3 + @DataProvider(name = "groupByCases") + public static Object[][] groupByCases() { return new Object[][]{ + // $.country — row0="US", row1="CA", row2=null, row3=missing → 2 of each + 4 null {"$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}}, "flat country"}, + // $.location.country — row0="US", row1=missing, row2=null parent, row3=missing → 2 "US" + 6 null {"$.location.country", new Object[][]{{"US", 2L}, {null, 6L}}, "nested country"}, - {"$.tags[0]", new Object[][]{{"green", 2L}, {"red", 2L}, {null, 4L}}, "array element"}, + // $.tags[0] — row0="red", row1="green", row2=null array, row3=missing → 2 each + 4 null + {"$.tags[0]", new Object[][]{{"green", 2L}, {"red", 2L}, {null, 4L}}, "array first element"}, + // $.events[0].country — row0="US", row1=null inner, row2=null array, row3=missing → 2 "US" + 6 null {"$.events[0].country", new Object[][]{{"US", 2L}, {null, 6L}}, "array-of-obj inner country"} }; } - @Test(dataProvider = "countryClickGroupByCases") - public void testCountryClickGroupByCountIncludesNullGroup(String jsonPath, Object[][] expectedRows, String label) { - String query = String.format( - "SELECT jsonExtractScalar(json, '%s', 'STRING') AS v, COUNT(*) FROM testTable " - + "GROUP BY v ORDER BY v ASC NULLS LAST", - jsonPath); - givenCountryClickFixture(true) - .whenQuery(query) - .thenResultIs(expectedRows); - } - /** - * Symmetric check: with null handling OFF, GROUP BY across mixed-resolution rows must throw - * (preserves legacy behavior). The broker surfaces the failure as a query error which the - * FluentQueryTest framework escalates to an {@link AssertionError}. + * GROUP BY with null handling OFF over the same mixed-resolution fixture must throw — the + * broker surfaces the failure as a query error which the FluentQueryTest framework escalates + * to an {@link AssertionError}. */ @Test - public void testCountryClickGroupByNullHandlingOffThrows() { + public void testGroupByOnUnresolvedPathThrowsWhenNullHandlingOff() { try { givenCountryClickFixture(false) .whenQuery("SELECT jsonExtractScalar(json, '$.country', 'STRING'), COUNT(*) FROM testTable GROUP BY 1") @@ -715,36 +826,44 @@ public void testCountryClickGroupByNullHandlingOffThrows() { } } + // ---------------- DISTINCT: null-handling ON must include null in the distinct set ---------------- + /** - * DISTINCT across paths with mixed resolution. With null handling ON, the distinct set must - * include exactly one null entry alongside the resolved values — null group must not blow up - * across segments (segment dedup), and must not throw. + * SELECT DISTINCT over the multi-row country/click fixture with null handling ON. The distinct + * set must include exactly one null entry alongside the resolved values — null must not blow up + * across segments, and must not throw. + *

+ * Example (flat country): + * 

+   *   Query: SELECT DISTINCT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
+   *          ORDER BY jsonExtractScalar(json, '$.country', 'STRING') ASC NULLS LAST
+   *
+   *   Result rows: ("CA"), ("US"), (NULL)
+   *
+ * Pinot requires the ORDER BY expression to match a DISTINCT column literally — positional + * ORDER BY (e.g. `ORDER BY 1`) is rejected with + * "ORDER-BY columns should be included in the DISTINCT columns" — hence the explicit expression. */ - @DataProvider(name = "countryClickDistinctCases") - public static Object[][] countryClickDistinctCases() { + @Test(dataProvider = "distinctCases") + public void testDistinctOnUnresolvedPathIncludesNull(String jsonPath, Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(json, '%s', 'STRING')", jsonPath); + String query = String.format("SELECT DISTINCT %s FROM testTable ORDER BY %s ASC NULLS LAST", expr, expr); + givenCountryClickFixture(true).whenQuery(query).thenResultIs(expectedRows); + } + + /** Each row is `(jsonPath, expected distinct values after ORDER BY v ASC NULLS LAST, label)`. */ + @DataProvider(name = "distinctCases") + public static Object[][] distinctCases() { return new Object[][]{ {"$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flat country"}, {"$.location.country", new Object[][]{{"US"}, {null}}, "nested country"}, - {"$.tags[0]", new Object[][]{{"green"}, {"red"}, {null}}, "array element"}, + {"$.tags[0]", new Object[][]{{"green"}, {"red"}, {null}}, "array first element"}, {"$.events[0].country", new Object[][]{{"US"}, {null}}, "array-of-obj inner country"} }; } - @Test(dataProvider = "countryClickDistinctCases") - public void testCountryClickDistinctIncludesNull(String jsonPath, Object[][] expectedRows, String label) { - // Pinot requires the ORDER BY expression to match a DISTINCT column literally — positional - // ORDER BY (e.g. ORDER BY 1) is rejected with "ORDER-BY columns should be included in the - // DISTINCT columns". - String expr = String.format("jsonExtractScalar(json, '%s', 'STRING')", jsonPath); - String query = String.format( - "SELECT DISTINCT %s FROM testTable ORDER BY %s ASC NULLS LAST", expr, expr); - givenCountryClickFixture(true) - .whenQuery(query) - .thenResultIs(expectedRows); - } - @Test - public void testCountryClickDistinctNullHandlingOffThrows() { + public void testDistinctOnUnresolvedPathThrowsWhenNullHandlingOff() { try { givenCountryClickFixture(false) .whenQuery("SELECT DISTINCT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable") From 34a3fba26d86df0f8a24a69f6169a8618e2bc2b8 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Fri, 22 May 2026 13:14:49 -0700 Subject: [PATCH 04/14] Reorganize country/click tests: single 2-column fixture, append-at-end Replace scattered single-row JSON constants with one 4-row x 2-column fixture (flatJson + nestedJson) appended to the end of the test class. Tests are grouped by query shape: projection -> DISTINCT -> GROUP BY, each with NH-on and NH-off variants. Per-test Javadocs include a concrete query and expected result; result rows are ordered ASC NULLS LAST. --- ...sonExtractScalarTransformFunctionTest.java | 730 ++++++++---------- 1 file changed, 304 insertions(+), 426 deletions(-) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index 56220a3fcb..bf3dc0b907 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -447,432 +447,6 @@ public void testIllegalArguments(String expressionStr) { TransformFunctionFactory.get(expression, _dataSourceMap); } - // ==================================================================================== - // === Country/click null-handling tests for jsonExtractScalar. === - // === === - // === The SV null-handling fix: with `enableNullHandling = true` and no default === - // === literal, an unresolved JSON path must surface as SQL NULL instead of throwing. === - // === These tests use a country/click data model — JSON documents shaped like === - // === `{country, clicks, location, tags, events}` — to exercise the fix across four === - // === JSON structural shapes (flat scalar, nested object, array element, array of === - // === objects) and three query shapes (projection, GROUP BY, DISTINCT). === - // ==================================================================================== - - // ---------------- Named single-row JSON documents (used by single-row tests) ---------------- - // - // Pretty-printed JSON for each constant is shown above its declaration. The string literal - // itself is kept compact — Jackson tolerates either form, but the comment above is what a - // human reads when scanning a test failure. - - // { "country": "US", "clicks": 5 } - private static final String CC_FLAT_RESOLVED = "{\"country\": \"US\", \"clicks\": 5}"; - // { "clicks": 5 } <- country key missing - private static final String CC_FLAT_COUNTRY_MISSING = "{\"clicks\": 5}"; - // { "country": null, "clicks": 5 } <- country explicit JSON null - private static final String CC_FLAT_COUNTRY_EXPLICIT_NULL = "{\"country\": null, \"clicks\": 5}"; - // { } <- empty document - private static final String CC_FLAT_EMPTY = "{}"; - // { "country": "US" } <- clicks key missing - private static final String CC_FLAT_CLICKS_MISSING = "{\"country\": \"US\"}"; - // { "country": "US", "clicks": null } <- clicks explicit JSON null - private static final String CC_FLAT_CLICKS_EXPLICIT_NULL = "{\"country\": \"US\", \"clicks\": null}"; - - // { "location": { "city": "SF", "country": "US" } } - private static final String CC_NESTED_RESOLVED = "{\"location\": {\"city\": \"SF\", \"country\": \"US\"}}"; - // { "location": { "city": "SF" } } <- nested inner key missing - private static final String CC_NESTED_INNER_MISSING = "{\"location\": {\"city\": \"SF\"}}"; - // { "location": null } <- nested parent explicit JSON null - private static final String CC_NESTED_PARENT_EXPLICIT_NULL = "{\"location\": null}"; - // { "country": "US" } <- nested parent itself missing - private static final String CC_NESTED_PARENT_MISSING = "{\"country\": \"US\"}"; - - // { "tags": ["red", "blue", "green"] } - private static final String CC_ARRAY_RESOLVED = "{\"tags\": [\"red\", \"blue\", \"green\"]}"; - // { "tags": null } <- array parent explicit JSON null - private static final String CC_ARRAY_NULL_PARENT = "{\"tags\": null}"; - // { "tags": [] } <- empty array - private static final String CC_ARRAY_EMPTY = "{\"tags\": []}"; - // { "tags": ["red"] } <- array index 1 out-of-bounds - private static final String CC_ARRAY_SINGLE_ELEM = "{\"tags\": [\"red\"]}"; - // { "country": "US" } <- array parent itself missing - private static final String CC_ARRAY_PARENT_MISSING = "{\"country\": \"US\"}"; - - // { "events": [{"country": "US"}, {"country": "CA"}] } - private static final String CC_AOO_RESOLVED = "{\"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"; - // { "events": [{"country": null}] } <- inner explicit JSON null - private static final String CC_AOO_INNER_EXPLICIT_NULL = "{\"events\": [{\"country\": null}]}"; - // { "events": [{}] } <- inner key missing - private static final String CC_AOO_INNER_KEY_MISSING = "{\"events\": [{}]}"; - // { "events": [] } <- empty array of objects - private static final String CC_AOO_EMPTY_ARRAY = "{\"events\": []}"; - // { "events": null } <- null array of objects - private static final String CC_AOO_NULL_ARRAY = "{\"events\": null}"; - // { "events": [{"country": "US"}] } <- inner numeric (clicks) missing - private static final String CC_AOO_INNER_NUMERIC_MISSING = "{\"events\": [{\"country\": \"US\"}]}"; - // { "events": [{"clicks": 1}, {"clicks": 7}] } - private static final String CC_AOO_NUMERIC_RESOLVED = "{\"events\": [{\"clicks\": 1}, {\"clicks\": 7}]}"; - - // ---------------- Multi-row fixture (used by GROUP BY / DISTINCT tests) ---------------- - // - // Pretty-printed JSON for each row: - // - // Row 0 — fully populated: - // { "country": "US", "clicks": 5, - // "location": { "city": "SF", "country": "US" }, - // "tags": ["red", "blue", "green"], - // "events": [{"country": "US"}, {"country": "CA"}] } - // - // Row 1 — partial: location has no country; events[0].country is explicit null: - // { "country": "CA", "clicks": 3, - // "location": { "city": "Tor" }, - // "tags": ["green"], - // "events": [{"country": null}] } - // - // Row 2 — every field explicit JSON null: - // { "country": null, "clicks": null, "location": null, "tags": null, "events": null } - // - // Row 3 — empty document: - // {} - // - // Across the four rows, every path covered by the GROUP BY / DISTINCT tests has at least one - // resolved value AND at least one unresolved occurrence (missing key or explicit null), so - // both the value groups and the null group must surface in the result. - - private static final String CC_ROW_0_FULL = "{\"country\": \"US\", \"clicks\": 5," - + " \"location\": {\"city\": \"SF\", \"country\": \"US\"}," - + " \"tags\": [\"red\", \"blue\", \"green\"]," - + " \"events\": [{\"country\": \"US\"}, {\"country\": \"CA\"}]}"; - - private static final String CC_ROW_1_PARTIAL = "{\"country\": \"CA\", \"clicks\": 3," - + " \"location\": {\"city\": \"Tor\"}," - + " \"tags\": [\"green\"]," - + " \"events\": [{\"country\": null}]}"; - - private static final String CC_ROW_2_ALL_NULL = - "{\"country\": null, \"clicks\": null, \"location\": null, \"tags\": null, \"events\": null}"; - - private static final String CC_ROW_3_EMPTY = "{}"; - - private static final Object[][] COUNTRY_CLICK_FIXTURE = { - {CC_ROW_0_FULL}, - {CC_ROW_1_PARTIAL}, - {CC_ROW_2_ALL_NULL}, - {CC_ROW_3_EMPTY} - }; - - // ---------------- Helpers ---------------- - - private FluentQueryTest.OnFirstInstance givenSingleRowJsonTable(boolean nullHandling, String json) { - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("testTable") - .setEnableColumnBasedNullHandling(true) - .addDimensionField("json", DataType.JSON) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - return FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(nullHandling) - .givenTable(schema, tableConfig) - .onFirstInstance(new Object[]{json}); - } - - private FluentQueryTest.OnFirstInstance givenCountryClickFixture(boolean nullHandling) { - Schema schema = new Schema.SchemaBuilder() - .setSchemaName("testTable") - .setEnableColumnBasedNullHandling(true) - .addDimensionField("json", DataType.JSON) - .build(); - TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("testTable").build(); - return FluentQueryTest.withBaseDir(_baseDir) - .withNullHandling(nullHandling) - .givenTable(schema, tableConfig) - .onFirstInstance(COUNTRY_CLICK_FIXTURE); - } - - /** Asserts that the 3-arg `jsonExtractScalar(...)` query returns SQL NULL for a single-row fixture. */ - private void assertSingleRowReturnsNull(boolean nullHandling, String json, String jsonPath, String resultsType) { - Object[] expectedNullRow = new Object[]{null}; - givenSingleRowJsonTable(nullHandling, json) - .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) - // Segment duplication: the same row is returned twice (one per duplicated segment). - .thenResultIs(expectedNullRow, expectedNullRow); - } - - /** Asserts that the 3-arg `jsonExtractScalar(...)` query throws when null handling is off. */ - private void assertSingleRowThrowsForUnresolved(String json, String jsonPath, String resultsType, String label) { - try { - givenSingleRowJsonTable(false, json) - .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) - .thenResultIs(new Object[]{"unused"}); - Assert.fail("Expected query to fail for " + label); - } catch (AssertionError e) { - Assertions.assertThat(e.getMessage()) - .describedAs("Case: " + label) - .contains("Cannot resolve JSON path on some records"); - } - } - - // ---------------- Unresolved paths: null-handling ON must surface SQL NULL ---------------- - - /** - * The SV null-handling fix. With null handling on and no default literal, an unresolved JSON - * path must surface as SQL NULL in the projection — matching the contract that - * {@code JsonIndexDistinctOperator} already provides for the DISTINCT route. - *

- * Example (flat, missing key): - * 

-   *   JSON:   { "clicks": 5 }
-   *   Query:  SET enableNullHandling = true;
-   *           SELECT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
-   *   Result: NULL
-   *
- * See {@link #unresolvedSinglePathCases()} for the full per-case matrix. - */ - @Test(dataProvider = "unresolvedSinglePathCases") - public void testNullHandlingOnReturnsNullForUnresolvedPath(String resultsType, String jsonPath, String json, - String label) { - assertSingleRowReturnsNull(true, json, jsonPath, resultsType); - } - - /** - * Legacy behavior preservation. With null handling off and no default, the same unresolved-path - * cases must throw — the E4 fix is gated on {@code _nullHandlingEnabled}, so callers that - * haven't opted in see no behavior change. - *

- * Example (flat, missing key): - * 

-   *   JSON:   { "clicks": 5 }
-   *   Query:  SELECT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
-   *   Result: throws IllegalArgumentException("Cannot resolve JSON path on some records.…")
-   *
- */ - @Test(dataProvider = "unresolvedSinglePathCases") - public void testNullHandlingOffThrowsForUnresolvedPath(String resultsType, String jsonPath, String json, - String label) { - assertSingleRowThrowsForUnresolved(json, jsonPath, resultsType, label); - } - - /** - * Each row is `(resultsType, jsonPath, JSON document, structural-shape label)`. Every case is - * one where `jsonPath` does NOT resolve in the given JSON document — that's what makes it - * exercise the null-handling code path. - */ - @DataProvider(name = "unresolvedSinglePathCases") - public static Object[][] unresolvedSinglePathCases() { - return new Object[][]{ - // FLAT — string path - {"STRING", "$.country", CC_FLAT_COUNTRY_MISSING, "flat: country key missing"}, - {"STRING", "$.country", CC_FLAT_COUNTRY_EXPLICIT_NULL, "flat: country explicit JSON null"}, - {"STRING", "$.country", CC_FLAT_EMPTY, "flat: empty document"}, - // FLAT — numeric path, all 5 numeric SV types - {"INT", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (INT)"}, - {"LONG", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (LONG)"}, - {"FLOAT", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (FLOAT)"}, - {"DOUBLE", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (DOUBLE)"}, - {"BIG_DECIMAL", "$.clicks", CC_FLAT_CLICKS_MISSING, "flat: clicks missing (BIG_DECIMAL)"}, - {"INT", "$.clicks", CC_FLAT_CLICKS_EXPLICIT_NULL, "flat: clicks explicit JSON null (INT)"}, - // NESTED OBJECT - {"STRING", "$.location.country", CC_NESTED_INNER_MISSING, "nested: inner key missing"}, - {"STRING", "$.location.country", CC_NESTED_PARENT_EXPLICIT_NULL, "nested: parent explicit JSON null"}, - {"STRING", "$.location.country", CC_NESTED_PARENT_MISSING, "nested: parent key missing"}, - // ARRAY ELEMENT - {"STRING", "$.tags[0]", CC_ARRAY_NULL_PARENT, "array: parent explicit JSON null"}, - {"STRING", "$.tags[0]", CC_ARRAY_EMPTY, "array: empty array"}, - {"STRING", "$.tags[1]", CC_ARRAY_SINGLE_ELEM, "array: index out-of-bounds"}, - {"STRING", "$.tags[0]", CC_ARRAY_PARENT_MISSING, "array: parent key missing"}, - // ARRAY OF OBJECTS - {"STRING", "$.events[0].country", CC_AOO_INNER_EXPLICIT_NULL, "array-of-obj: inner explicit JSON null"}, - {"STRING", "$.events[0].country", CC_AOO_INNER_KEY_MISSING, "array-of-obj: inner key missing"}, - {"STRING", "$.events[0].country", CC_AOO_EMPTY_ARRAY, "array-of-obj: empty array"}, - {"STRING", "$.events[0].country", CC_AOO_NULL_ARRAY, "array-of-obj: null array"}, - // ARRAY OF OBJECTS — numeric (INT) confirms typed coverage at depth - {"INT", "$.events[0].clicks", CC_AOO_INNER_NUMERIC_MISSING, "array-of-obj: inner numeric missing (INT)"} - }; - } - - // ---------------- Resolved paths: null-handling ON must pass values through ---------------- - - /** - * Resolved-path regression guard. The fix only changes the unresolved-row behavior; resolved - * paths must still produce their values verbatim at every JSON nesting depth. - *

- * Example (array of objects, numeric): - * 

-   *   JSON:   { "events": [{"clicks": 1}, {"clicks": 7}] }
-   *   Query:  SET enableNullHandling = true;
-   *           SELECT jsonExtractScalar(json, '$.events[1].clicks', 'INT') FROM testTable
-   *   Result: 7
-   *
- */ - @Test(dataProvider = "resolvedSinglePathCases") - public void testNullHandlingOnPassesResolvedValueThrough(String resultsType, String jsonPath, String json, - Object expectedValue, String label) { - Object[] expectedRow = new Object[]{expectedValue}; - givenSingleRowJsonTable(true, json) - .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', '%s') FROM testTable", jsonPath, resultsType)) - .thenResultIs(expectedRow, expectedRow); - } - - /** Each row is `(resultsType, jsonPath, JSON document, expected resolved value, label)`. */ - @DataProvider(name = "resolvedSinglePathCases") - public static Object[][] resolvedSinglePathCases() { - return new Object[][]{ - {"STRING", "$.country", CC_FLAT_RESOLVED, "US", "flat string"}, - {"INT", "$.clicks", CC_FLAT_RESOLVED, 5, "flat int"}, - {"STRING", "$.location.country", CC_NESTED_RESOLVED, "US", "nested string"}, - {"STRING", "$.tags[1]", CC_ARRAY_RESOLVED, "blue", "array element"}, - {"STRING", "$.events[0].country", CC_AOO_RESOLVED, "US", "array-of-obj inner string"}, - {"INT", "$.events[1].clicks", CC_AOO_NUMERIC_RESOLVED, 7, "array-of-obj inner numeric"} - }; - } - - // ---------------- 4-arg with SQL NULL default + null-handling ON (regression guard) ---------------- - - /** - * 4-arg `jsonExtractScalar(json, path, type, NULL)` + null handling ON. This is the pre-existing - * {@code _defaultIsNull} code path — must continue producing SQL NULL at every nesting depth so - * the E4 fix doesn't disturb it. - *

- * Example (nested, inner missing): - * 

-   *   JSON:   { "location": { "city": "SF" } }
-   *   Query:  SET enableNullHandling = true;
-   *           SELECT jsonExtractScalar(json, '$.location.country', 'STRING', NULL) FROM testTable
-   *   Result: NULL
-   *
- */ - @Test(dataProvider = "fourArgSqlNullDefaultCases") - public void testFourArgSqlNullDefaultReturnsNullWithNullHandlingOn(String jsonPath, String json, String label) { - Object[] expectedRow = new Object[]{null}; - givenSingleRowJsonTable(true, json) - .whenQuery(String.format("SELECT jsonExtractScalar(json, '%s', 'STRING', NULL) FROM testTable", jsonPath)) - .thenResultIs(expectedRow, expectedRow); - } - - /** Each row is `(jsonPath, JSON document, label)` — all rows pick a path that does NOT resolve. */ - @DataProvider(name = "fourArgSqlNullDefaultCases") - public static Object[][] fourArgSqlNullDefaultCases() { - return new Object[][]{ - {"$.country", CC_FLAT_COUNTRY_MISSING, "flat: key missing"}, - {"$.location.country", CC_NESTED_INNER_MISSING, "nested: inner missing"}, - {"$.location.country", CC_NESTED_PARENT_EXPLICIT_NULL, "nested: parent explicit JSON null"}, - {"$.tags[0]", CC_ARRAY_EMPTY, "array: empty"}, - {"$.tags[0]", CC_ARRAY_NULL_PARENT, "array: null parent"}, - {"$.events[0].country", CC_AOO_INNER_EXPLICIT_NULL, "array-of-obj: inner explicit JSON null"}, - {"$.events[0].country", CC_FLAT_EMPTY, "deep path against empty root"} - }; - } - - // ---------------- GROUP BY: null-handling ON must produce a single null group ---------------- - - /** - * GROUP BY over the multi-row country/click fixture, with null handling ON. The unresolved rows - * (per the row breakdown in the fixture Javadoc above) must collapse into a single null group - * with the correct count, alongside the resolved value groups. - *

- * Example (flat country): - * 

-   *   Rows (×2 for segment duplication):
-   *     Row 0: country = "US"        Row 1: country = "CA"
-   *     Row 2: country = JSON null   Row 3: country missing
-   *
-   *   Query: SELECT jsonExtractScalar(json, '$.country', 'STRING') AS v, COUNT(*)
-   *          FROM testTable GROUP BY v ORDER BY v ASC NULLS LAST
-   *
-   *   Result rows: ("CA", 2), ("US", 2), (NULL, 4)
-   *
- * The data provider lists the expected (value, count) rows for each path covered. - */ - @Test(dataProvider = "groupByCases") - public void testGroupByOnUnresolvedPathProducesNullGroup(String jsonPath, Object[][] expectedRows, String label) { - String query = String.format( - "SELECT jsonExtractScalar(json, '%s', 'STRING') AS v, COUNT(*) FROM testTable " - + "GROUP BY v ORDER BY v ASC NULLS LAST", - jsonPath); - givenCountryClickFixture(true).whenQuery(query).thenResultIs(expectedRows); - } - - /** - * Each row is `(jsonPath, expected (value, count) rows after ORDER BY v ASC NULLS LAST, label)`. - * Counts are ×2 the per-row count due to segment duplication (one segment becomes two). - */ - @DataProvider(name = "groupByCases") - public static Object[][] groupByCases() { - return new Object[][]{ - // $.country — row0="US", row1="CA", row2=null, row3=missing → 2 of each + 4 null - {"$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}}, "flat country"}, - // $.location.country — row0="US", row1=missing, row2=null parent, row3=missing → 2 "US" + 6 null - {"$.location.country", new Object[][]{{"US", 2L}, {null, 6L}}, "nested country"}, - // $.tags[0] — row0="red", row1="green", row2=null array, row3=missing → 2 each + 4 null - {"$.tags[0]", new Object[][]{{"green", 2L}, {"red", 2L}, {null, 4L}}, "array first element"}, - // $.events[0].country — row0="US", row1=null inner, row2=null array, row3=missing → 2 "US" + 6 null - {"$.events[0].country", new Object[][]{{"US", 2L}, {null, 6L}}, "array-of-obj inner country"} - }; - } - - /** - * GROUP BY with null handling OFF over the same mixed-resolution fixture must throw — the - * broker surfaces the failure as a query error which the FluentQueryTest framework escalates - * to an {@link AssertionError}. - */ - @Test - public void testGroupByOnUnresolvedPathThrowsWhenNullHandlingOff() { - try { - givenCountryClickFixture(false) - .whenQuery("SELECT jsonExtractScalar(json, '$.country', 'STRING'), COUNT(*) FROM testTable GROUP BY 1") - .thenResultIs(new Object[]{"unused"}); - Assert.fail("Expected GROUP BY query to fail when null handling is off and rows are unresolved"); - } catch (AssertionError e) { - Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); - } - } - - // ---------------- DISTINCT: null-handling ON must include null in the distinct set ---------------- - - /** - * SELECT DISTINCT over the multi-row country/click fixture with null handling ON. The distinct - * set must include exactly one null entry alongside the resolved values — null must not blow up - * across segments, and must not throw. - *

- * Example (flat country): - * 

-   *   Query: SELECT DISTINCT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable
-   *          ORDER BY jsonExtractScalar(json, '$.country', 'STRING') ASC NULLS LAST
-   *
-   *   Result rows: ("CA"), ("US"), (NULL)
-   *
- * Pinot requires the ORDER BY expression to match a DISTINCT column literally — positional - * ORDER BY (e.g. `ORDER BY 1`) is rejected with - * "ORDER-BY columns should be included in the DISTINCT columns" — hence the explicit expression. - */ - @Test(dataProvider = "distinctCases") - public void testDistinctOnUnresolvedPathIncludesNull(String jsonPath, Object[][] expectedRows, String label) { - String expr = String.format("jsonExtractScalar(json, '%s', 'STRING')", jsonPath); - String query = String.format("SELECT DISTINCT %s FROM testTable ORDER BY %s ASC NULLS LAST", expr, expr); - givenCountryClickFixture(true).whenQuery(query).thenResultIs(expectedRows); - } - - /** Each row is `(jsonPath, expected distinct values after ORDER BY v ASC NULLS LAST, label)`. */ - @DataProvider(name = "distinctCases") - public static Object[][] distinctCases() { - return new Object[][]{ - {"$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flat country"}, - {"$.location.country", new Object[][]{{"US"}, {null}}, "nested country"}, - {"$.tags[0]", new Object[][]{{"green"}, {"red"}, {null}}, "array first element"}, - {"$.events[0].country", new Object[][]{{"US"}, {null}}, "array-of-obj inner country"} - }; - } - - @Test - public void testDistinctOnUnresolvedPathThrowsWhenNullHandlingOff() { - try { - givenCountryClickFixture(false) - .whenQuery("SELECT DISTINCT jsonExtractScalar(json, '$.country', 'STRING') FROM testTable") - .thenResultIs(new Object[]{"unused"}); - Assert.fail("Expected DISTINCT query to fail when null handling is off and rows are unresolved"); - } catch (AssertionError e) { - Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); - } - } @Test(dataProvider = "testParsingIllegalQueries", expectedExceptions = {SqlCompilationException.class}) public void testParsingIllegalQueries(String expressionStr) { @@ -1330,4 +904,308 @@ public void testCrossTypeConversionFromStringResult() { .whenQuery("SELECT CAST(jsonExtractScalar(json, '$.v', 'STRING') AS LONG) FROM testTable") .thenResultIs(new Object[]{42L}, new Object[]{42L}); } + + // ============================================================================================ + // Country/click null-handling tests for jsonExtractScalar (issue #18568). + // + // The SV null-handling fix: with `enableNullHandling = true` and no default literal, an + // unresolved JSON path must surface as SQL NULL instead of throwing. + // + // All tests below share a single `clicks` table fixture with two JSON columns and four rows: + // + // `flatJson` — 1-level scalar shapes. Each row exercises one case: resolved value, explicit + // JSON null, missing key, empty document. + // `nestedJson` — multi-level shapes (nested object, array, array of objects) with mixed + // resolved / null / missing data at depth. + // + // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each group + // has a null-handling-on case (verifies SQL NULL surfaces) and a null-handling-off case + // (verifies the legacy throw is preserved). Result rows in every assertion are ordered by the + // projected expression ASC NULLS LAST so the comparison is deterministic across the framework's + // segment-duplication behavior (a single segment shows up twice, so per-row counts are ×2). + // ============================================================================================ + + // ---------------- Fixture ---------------- + // + // Pretty-printed contents of the 4-row × 2-column fixture: + // + // row 0 — fully populated: + // flatJson: { "country": "US", "clicks": 5 } + // nestedJson: { "location": {"city": "SF", "country": "US"}, + // "tags": ["red", "blue", "green"], + // "events": [{"country": "US"}, {"country": "CA"}] } + // + // row 1 — partial / explicit-null at depth: + // flatJson: { "country": "CA", "clicks": 3 } + // nestedJson: { "location": {"city": "Tor"}, // <-- no country key + // "tags": ["green"], // <-- only one element + // "events": [{"country": null}] } // <-- inner explicit null + // + // row 2 — every field explicit JSON null: + // flatJson: { "country": null, "clicks": null } + // nestedJson: { "location": null, "tags": null, "events": null } + // + // row 3 — empty documents: + // flatJson: {} + // nestedJson: {} + // + // Across the four rows, every path covered by the tests has at least one resolved value AND + // at least one unresolved occurrence (missing key, explicit JSON null, null parent, or + // out-of-bounds array index), so both the value groups and the null group must show up. + + private static final Object[][] COUNTRY_CLICK_FIXTURE = { + // {flatJson, nestedJson} + { + "{\"country\":\"US\",\"clicks\":5}", + "{\"location\":{\"city\":\"SF\",\"country\":\"US\"}," + + "\"tags\":[\"red\",\"blue\",\"green\"]," + + "\"events\":[{\"country\":\"US\"},{\"country\":\"CA\"}]}" + }, + { + "{\"country\":\"CA\",\"clicks\":3}", + "{\"location\":{\"city\":\"Tor\"}," + + "\"tags\":[\"green\"]," + + "\"events\":[{\"country\":null}]}" + }, + { + "{\"country\":null,\"clicks\":null}", + "{\"location\":null,\"tags\":null,\"events\":null}" + }, + {"{}", "{}"} + }; + + // ---------------- Helper ---------------- + + private FluentQueryTest.OnFirstInstance givenCountryClickTable(boolean nullHandling) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("clicks") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("flatJson", DataType.JSON) + .addDimensionField("nestedJson", DataType.JSON) + .build(); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("clicks").build(); + return FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(nullHandling) + .givenTable(schema, tableConfig) + .onFirstInstance(COUNTRY_CLICK_FIXTURE); + } + + // ============================================================================================ + // 1. Projection + // ============================================================================================ + + /** + * Projection with null handling ON. Each row's resolution depends on the column + path; the SV + * transform must surface SQL NULL for unresolved rows and pass resolved values through. + * 
+   *   Example — `flatJson.country` (STRING):
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
+   *
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT jsonExtractScalar(flatJson, '$.country', 'STRING') AS c
+   *             FROM clicks ORDER BY c ASC NULLS LAST
+   *
+   *     Result rows (×2 for segment duplication):
+   *             "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
+   *
+ */ + @Test(dataProvider = "projectionCases") + public void testProjectionNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', '%s')", column, jsonPath, resultsType); + givenCountryClickTable(true) + .whenQuery(String.format("SELECT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr)) + .thenResultIs(expectedRows); + } + + /** + * Null handling OFF: any unresolved row throws. One representative path is enough — the throw + * is independent of path shape and result type. + */ + @Test + public void testProjectionNullHandlingOffThrows() { + try { + givenCountryClickTable(false) + .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING') FROM clicks") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected projection to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); + } + } + + /** + * Each row is `(column, jsonPath, resultsType, expected 8-row result, label)`. Result rows are + * ordered by value ASC NULLS LAST. With segment duplication, the per-row count in the fixture + * is ×2 in the result. + */ + @DataProvider(name = "projectionCases") + public static Object[][] projectionCases() { + return new Object[][]{ + // ----- flatJson: 1-level scalar shape ----- + // $.country: row0="US", row1="CA", row2=null (explicit), row3=null (missing). + {"flatJson", "$.country", "STRING", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null} + }, "flatJson.country STRING"}, + // $.clicks across all 6 numeric SV types: row0=5, row1=3, row2=null, row3=null. + {"flatJson", "$.clicks", "INT", new Object[][]{ + {3}, {3}, {5}, {5}, {null}, {null}, {null}, {null} + }, "flatJson.clicks INT"}, + {"flatJson", "$.clicks", "LONG", new Object[][]{ + {3L}, {3L}, {5L}, {5L}, {null}, {null}, {null}, {null} + }, "flatJson.clicks LONG"}, + {"flatJson", "$.clicks", "FLOAT", new Object[][]{ + {3f}, {3f}, {5f}, {5f}, {null}, {null}, {null}, {null} + }, "flatJson.clicks FLOAT"}, + {"flatJson", "$.clicks", "DOUBLE", new Object[][]{ + {3d}, {3d}, {5d}, {5d}, {null}, {null}, {null}, {null} + }, "flatJson.clicks DOUBLE"}, + // BIG_DECIMAL is formatted as String by the broker (BigDecimal.toPlainString). + {"flatJson", "$.clicks", "BIG_DECIMAL", new Object[][]{ + {"3"}, {"3"}, {"5"}, {"5"}, {null}, {null}, {null}, {null} + }, "flatJson.clicks BIG_DECIMAL"}, + + // ----- nestedJson: multi-level shapes ----- + // $.location.country: row0="US", row1=missing (no country key), row2=parent null, row3=missing. + {"nestedJson", "$.location.country", "STRING", new Object[][]{ + {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.location.country STRING"}, + // $.tags[0]: row0="red", row1="green", row2=null parent, row3=missing. + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{ + {"green"}, {"green"}, {"red"}, {"red"}, {null}, {null}, {null}, {null} + }, "nestedJson.tags[0] STRING"}, + // $.tags[1]: row0="blue", row1=out-of-bounds (only one element), row2=null parent, row3=missing. + {"nestedJson", "$.tags[1]", "STRING", new Object[][]{ + {"blue"}, {"blue"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.tags[1] STRING (OOB on row 1)"}, + // $.events[0].country: row0="US", row1=explicit inner null, row2=null array, row3=missing. + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{ + {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.events[0].country STRING"} + }; + } + + // ============================================================================================ + // 2. DISTINCT + // ============================================================================================ + + /** + * DISTINCT with null handling ON. The distinct set must include exactly one null entry + * alongside the resolved values (deduped across segments). + * 
+   *   Example — `flatJson.country` (STRING):
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT DISTINCT jsonExtractScalar(flatJson, '$.country', 'STRING') FROM clicks
+   *             ORDER BY jsonExtractScalar(flatJson, '$.country', 'STRING') ASC NULLS LAST
+   *
+   *     Result: "CA", "US", NULL
+   *
+ * Pinot rejects positional `ORDER BY 1` for DISTINCT ("ORDER-BY columns should be included in + * the DISTINCT columns"), so the expression is repeated in the ORDER BY clause. + */ + @Test(dataProvider = "distinctCases") + public void testDistinctNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', '%s')", column, jsonPath, resultsType); + String query = String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr); + givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); + } + + @Test + public void testDistinctNullHandlingOffThrows() { + try { + givenCountryClickTable(false) + .whenQuery("SELECT DISTINCT jsonExtractScalar(flatJson, '$.country', 'STRING') FROM clicks") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected DISTINCT to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); + } + } + + /** Each row is `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */ + @DataProvider(name = "distinctCases") + public static Object[][] distinctCases() { + return new Object[][]{ + // flatJson: 1-level scalar. + {"flatJson", "$.country", "STRING", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country"}, + {"flatJson", "$.clicks", "INT", new Object[][]{{3}, {5}, {null}}, "flatJson.clicks INT"}, + // nestedJson: multi-level paths. + {"nestedJson", "$.location.country", "STRING", new Object[][]{{"US"}, {null}}, + "nestedJson.location.country"}, + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{{"green"}, {"red"}, {null}}, + "nestedJson.tags[0]"}, + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{{"US"}, {null}}, + "nestedJson.events[0].country"} + }; + } + + // ============================================================================================ + // 3. GROUP BY + // ============================================================================================ + + /** + * GROUP BY with null handling ON. Unresolved rows must collapse into a single null group with + * the correct count, alongside the resolved value groups. + * 
+   *   Example — `flatJson.country` (STRING):
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
+   *
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT jsonExtractScalar(flatJson, '$.country', 'STRING') AS v, COUNT(*)
+   *             FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST
+   *
+   *     Result (counts ×2 for segment duplication): ("CA", 2), ("US", 2), (NULL, 4)
+   *
+ */ + @Test(dataProvider = "groupByCases") + public void testGroupByNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', '%s')", column, jsonPath, resultsType); + String query = String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr); + givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); + } + + @Test + public void testGroupByNullHandlingOffThrows() { + try { + givenCountryClickTable(false) + .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING'), COUNT(*) FROM clicks GROUP BY 1") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected GROUP BY to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Cannot resolve JSON path on some records"); + } + } + + /** + * Each row is `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST, + * label)`. Counts are ×2 the per-row count due to segment duplication. + */ + @DataProvider(name = "groupByCases") + public static Object[][] groupByCases() { + return new Object[][]{ + // flatJson.country: row0="US", row1="CA", row2=null, row3=null -> 2 each + 4 null. + {"flatJson", "$.country", "STRING", new Object[][]{ + {"CA", 2L}, {"US", 2L}, {null, 4L} + }, "flatJson.country"}, + // flatJson.clicks INT: row0=5, row1=3, row2=null, row3=null -> 2 each + 4 null. + {"flatJson", "$.clicks", "INT", new Object[][]{ + {3, 2L}, {5, 2L}, {null, 4L} + }, "flatJson.clicks INT"}, + // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 null. + {"nestedJson", "$.location.country", "STRING", new Object[][]{ + {"US", 2L}, {null, 6L} + }, "nestedJson.location.country"}, + // nestedJson.tags[0]: row0="red", row1="green", rows 2/3 unresolved -> 2 each + 4 null. + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{ + {"green", 2L}, {"red", 2L}, {null, 4L} + }, "nestedJson.tags[0]"}, + // nestedJson.events[0].country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 null. + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{ + {"US", 2L}, {null, 6L} + }, "nestedJson.events[0].country"} + }; + } } From b54e66056badcbc6e00527f880245416d9b206aa Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Fri, 22 May 2026 14:48:18 -0700 Subject: [PATCH 05/14] Mirror country/click test structure in JsonExtractIndexTransformFunctionTest Replace the minimal unit-level null-handling tests with the same fixture + projection/DISTINCT/GROUP BY structure as the scalar test, so both test files cover the same query shapes against the same data model. The helper configures a JSON index on both columns via FieldConfig. --- ...JsonExtractIndexTransformFunctionTest.java | 428 ++++++++++++++---- 1 file changed, 331 insertions(+), 97 deletions(-) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java index d494332e25..e88b7e94f7 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java @@ -18,6 +18,8 @@ */ package org.apache.pinot.core.operator.transform.function; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import com.jayway.jsonpath.Configuration; import com.jayway.jsonpath.JsonPath; import com.jayway.jsonpath.Option; @@ -25,17 +27,29 @@ import com.jayway.jsonpath.TypeRef; import com.jayway.jsonpath.spi.json.JacksonJsonProvider; import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider; +import java.io.File; +import java.io.IOException; +import java.io.UncheckedIOException; import java.math.BigDecimal; +import java.nio.file.Files; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import org.apache.commons.io.FileUtils; import org.apache.pinot.common.function.JsonPathCache; import org.apache.pinot.common.request.context.ExpressionContext; import org.apache.pinot.common.request.context.RequestContextUtils; +import org.apache.pinot.queries.FluentQueryTest; +import org.apache.pinot.spi.config.table.FieldConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.config.table.TableType; import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.spi.utils.builder.TableConfigBuilder; import org.assertj.core.api.Assertions; -import org.roaringbitmap.RoaringBitmap; import org.testng.Assert; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -426,117 +440,337 @@ private T getValueForKey(String blob, JsonPath path, TypeRef typeRef) { return JSON_PARSER_CONTEXT.parse(blob).read(path, typeRef); } - // === Null-handling contract for unresolved JSON paths === + // ============================================================================================ + // Country/click null-handling tests for jsonExtractIndex (issue #18568). // - // The base fixture's JSON_STRING_SV_COLUMN has no '$.noField' key in any row, so the JSON-index - // lookup returns null for every doc. With query-level null handling ENABLED, the SV transforms - // must surface those rows as SQL NULL (type-default + a bit in getNullBitmap()), matching - // JsonIndexDistinctOperator's null-group semantics. With null handling DISABLED and no default - // literal, the legacy RuntimeException must be preserved. - - @DataProvider(name = "unresolvedPathSvTypes") - public Object[][] unresolvedPathSvTypes() { + // The SV null-handling fix: with `enableNullHandling = true` and no default literal, an + // unresolved JSON path must surface as SQL NULL instead of throwing. + // + // All tests below share a single `clicks` table fixture with two JSON-indexed columns and four + // rows. The fixture mirrors the one in JsonExtractScalarTransformFunctionTest so that, modulo + // the function name, expected results are identical — `jsonExtractIndex` and + // `jsonExtractScalar` must agree on null semantics after the fix. + // + // `flatJson` — 1-level scalar shapes. Each row exercises one case: resolved value, explicit + // JSON null, missing key, empty document. + // `nestedJson` — multi-level shapes (nested object, array, array of objects) with mixed + // resolved / null / missing data at depth. + // + // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each group + // has a null-handling-on case (verifies SQL NULL surfaces) and a null-handling-off case + // (verifies the legacy throw is preserved). Result rows are ordered by the projected expression + // ASC NULLS LAST so the comparison is deterministic across the framework's segment-duplication + // behavior (one segment becomes two, so per-row counts in GROUP BY results are ×2). + // ============================================================================================ + + protected File _baseDir; + + @BeforeClass + void createBaseDir() { + try { + _baseDir = Files.createTempDirectory(getClass().getSimpleName()).toFile(); + } catch (IOException ex) { + throw new UncheckedIOException(ex); + } + } + + @AfterClass + void destroyBaseDir() + throws IOException { + if (_baseDir != null) { + FileUtils.deleteDirectory(_baseDir); + } + } + + // ---------------- Fixture ---------------- + // + // Pretty-printed contents of the 4-row × 2-column fixture: + // + // row 0 — fully populated: + // flatJson: { "country": "US", "clicks": 5 } + // nestedJson: { "location": {"city": "SF", "country": "US"}, + // "tags": ["red", "blue", "green"], + // "events": [{"country": "US"}, {"country": "CA"}] } + // + // row 1 — partial / explicit-null at depth: + // flatJson: { "country": "CA", "clicks": 3 } + // nestedJson: { "location": {"city": "Tor"}, // <-- no country key + // "tags": ["green"], // <-- only one element + // "events": [{"country": null}] } // <-- inner explicit null + // + // row 2 — every field explicit JSON null: + // flatJson: { "country": null, "clicks": null } + // nestedJson: { "location": null, "tags": null, "events": null } + // + // row 3 — empty documents: + // flatJson: {} + // nestedJson: {} + + private static final Object[][] COUNTRY_CLICK_FIXTURE = { + // {flatJson, nestedJson} + { + "{\"country\":\"US\",\"clicks\":5}", + "{\"location\":{\"city\":\"SF\",\"country\":\"US\"}," + + "\"tags\":[\"red\",\"blue\",\"green\"]," + + "\"events\":[{\"country\":\"US\"},{\"country\":\"CA\"}]}" + }, + { + "{\"country\":\"CA\",\"clicks\":3}", + "{\"location\":{\"city\":\"Tor\"}," + + "\"tags\":[\"green\"]," + + "\"events\":[{\"country\":null}]}" + }, + { + "{\"country\":null,\"clicks\":null}", + "{\"location\":null,\"tags\":null,\"events\":null}" + }, + {"{}", "{}"} + }; + + // ---------------- Helper ---------------- + // + // `jsonExtractIndex` requires the column to have a JSON index. The helper wires a JSON index + // onto both columns via FieldConfig — same shape as the JSON index in + // BaseTransformFunctionTest.getTableConfig(). + + private FluentQueryTest.OnFirstInstance givenCountryClickTable(boolean nullHandling) { + Schema schema = new Schema.SchemaBuilder() + .setSchemaName("clicks") + .setEnableColumnBasedNullHandling(true) + .addDimensionField("flatJson", DataType.JSON) + .addDimensionField("nestedJson", DataType.JSON) + .build(); + ObjectNode jsonIndexNode = JsonNodeFactory.instance.objectNode(); + jsonIndexNode.set("json", JsonNodeFactory.instance.objectNode()); + List fieldConfigs = List.of( + new FieldConfig("flatJson", FieldConfig.EncodingType.DICTIONARY, null, null, null, null, jsonIndexNode, null, + null), + new FieldConfig("nestedJson", FieldConfig.EncodingType.DICTIONARY, null, null, null, null, jsonIndexNode, null, + null)); + TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE) + .setTableName("clicks") + .setFieldConfigList(fieldConfigs) + .build(); + return FluentQueryTest.withBaseDir(_baseDir) + .withNullHandling(nullHandling) + .givenTable(schema, tableConfig) + .onFirstInstance(COUNTRY_CLICK_FIXTURE); + } + + // ============================================================================================ + // 1. Projection + // ============================================================================================ + + /** + * Projection with null handling ON. Each row's resolution depends on the column + path; the SV + * transform must surface SQL NULL for unresolved rows and pass resolved values through. + * 
+   *   Example — `flatJson.country` (STRING):
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
+   *
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT jsonExtractIndex(flatJson, '$.country', 'STRING') AS c
+   *             FROM clicks ORDER BY c ASC NULLS LAST
+   *
+   *     Result rows (×2 for segment duplication):
+   *             "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
+   *
+ */ + @Test(dataProvider = "projectionCases") + public void testProjectionNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractIndex(%s, '%s', '%s')", column, jsonPath, resultsType); + givenCountryClickTable(true) + .whenQuery(String.format("SELECT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr)) + .thenResultIs(expectedRows); + } + + /** + * Null handling OFF: any unresolved row throws (legacy behavior, preserved by the fix's + * `_nullHandlingEnabled` gate). One representative path is enough — the throw is independent of + * path shape and result type. The error message for `jsonExtractIndex` differs from the scalar + * function's: `"Illegal Json Path: [...], for docId [...]"`. + */ + @Test + public void testProjectionNullHandlingOffThrows() { + try { + givenCountryClickTable(false) + .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING') FROM clicks") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected projection to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Illegal Json Path"); + } + } + + /** + * Each row is `(column, jsonPath, resultsType, expected 8-row result, label)`. Result rows are + * ordered by value ASC NULLS LAST. With segment duplication, the per-row count in the fixture + * is ×2 in the result. + */ + @DataProvider(name = "projectionCases") + public static Object[][] projectionCases() { return new Object[][]{ - {"INT", DataType.INT}, - {"LONG", DataType.LONG}, - {"FLOAT", DataType.FLOAT}, - {"DOUBLE", DataType.DOUBLE}, - {"BIG_DECIMAL", DataType.BIG_DECIMAL}, - {"STRING", DataType.STRING} + // ----- flatJson: 1-level scalar shape ----- + // $.country: row0="US", row1="CA", row2=null (explicit), row3=null (missing). + {"flatJson", "$.country", "STRING", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null} + }, "flatJson.country STRING"}, + // $.clicks across all 6 numeric SV types: row0=5, row1=3, row2=null, row3=null. + {"flatJson", "$.clicks", "INT", new Object[][]{ + {3}, {3}, {5}, {5}, {null}, {null}, {null}, {null} + }, "flatJson.clicks INT"}, + {"flatJson", "$.clicks", "LONG", new Object[][]{ + {3L}, {3L}, {5L}, {5L}, {null}, {null}, {null}, {null} + }, "flatJson.clicks LONG"}, + {"flatJson", "$.clicks", "FLOAT", new Object[][]{ + {3f}, {3f}, {5f}, {5f}, {null}, {null}, {null}, {null} + }, "flatJson.clicks FLOAT"}, + {"flatJson", "$.clicks", "DOUBLE", new Object[][]{ + {3d}, {3d}, {5d}, {5d}, {null}, {null}, {null}, {null} + }, "flatJson.clicks DOUBLE"}, + // BIG_DECIMAL is formatted as String by the broker (BigDecimal.toPlainString). + {"flatJson", "$.clicks", "BIG_DECIMAL", new Object[][]{ + {"3"}, {"3"}, {"5"}, {"5"}, {null}, {null}, {null}, {null} + }, "flatJson.clicks BIG_DECIMAL"}, + + // ----- nestedJson: multi-level shapes ----- + // $.location.country: row0="US", row1=missing (no country key), row2=parent null, row3=missing. + {"nestedJson", "$.location.country", "STRING", new Object[][]{ + {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.location.country STRING"}, + // $.tags[0]: row0="red", row1="green", row2=null parent, row3=missing. + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{ + {"green"}, {"green"}, {"red"}, {"red"}, {null}, {null}, {null}, {null} + }, "nestedJson.tags[0] STRING"}, + // $.tags[1]: row0="blue", row1=out-of-bounds (only one element), row2=null parent, row3=missing. + {"nestedJson", "$.tags[1]", "STRING", new Object[][]{ + {"blue"}, {"blue"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.tags[1] STRING (OOB on row 1)"}, + // $.events[0].country: row0="US", row1=explicit inner null, row2=null array, row3=missing. + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{ + {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.events[0].country STRING"} }; } - @Test(dataProvider = "unresolvedPathSvTypes") - public void testNullHandlingEnabledEmitsNullForUnresolvedPath(String resultsType, DataType resultsDataType) { - String expressionStr = String.format("jsonExtractIndex(%s,'$.noField','%s')", JSON_STRING_SV_COLUMN, resultsType); - ExpressionContext expression = RequestContextUtils.getExpression(expressionStr); - TransformFunction transformFunction = - TransformFunctionFactory.getNullHandlingEnabled(expression, _dataSourceMap); + // ============================================================================================ + // 2. DISTINCT + // ============================================================================================ - Assert.assertTrue(transformFunction instanceof JsonExtractIndexTransformFunction); - Assert.assertEquals(transformFunction.getResultMetadata().getDataType(), resultsDataType); + /** + * DISTINCT with null handling ON. The distinct set must include exactly one null entry + * alongside the resolved values (deduped across segments). + * 
+   *   Example — `flatJson.country` (STRING):
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT DISTINCT jsonExtractIndex(flatJson, '$.country', 'STRING') FROM clicks
+   *             ORDER BY jsonExtractIndex(flatJson, '$.country', 'STRING') ASC NULLS LAST
+   *
+   *     Result: "CA", "US", NULL
+   *
+ * For STRING DISTINCT against a JSON-indexed column, the broker actually routes the query + * through {@code JsonIndexDistinctOperator} (not the transform-fn path) — so this case + * indirectly exercises that operator. The other types fall back to the transform-fn path + * because the operator's parser converts the default literal in a way that fails for + * non-string types; that fallback is what this fix corrects. + */ + @Test(dataProvider = "distinctCases") + public void testDistinctNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractIndex(%s, '%s', '%s')", column, jsonPath, resultsType); + String query = String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr); + givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); + } - switch (resultsDataType) { - case INT: - int[] intValues = transformFunction.transformToIntValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(intValues[i], 0); - } - break; - case LONG: - long[] longValues = transformFunction.transformToLongValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(longValues[i], 0L); - } - break; - case FLOAT: - float[] floatValues = transformFunction.transformToFloatValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(floatValues[i], 0f); - } - break; - case DOUBLE: - double[] doubleValues = transformFunction.transformToDoubleValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(doubleValues[i], 0d); - } - break; - case BIG_DECIMAL: - BigDecimal[] bigDecimalValues = transformFunction.transformToBigDecimalValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(bigDecimalValues[i].compareTo(BigDecimal.ZERO), 0); - } - break; - case STRING: - String[] stringValues = transformFunction.transformToStringValuesSV(_projectionBlock); - for (int i = 0; i < NUM_ROWS; i++) { - Assert.assertEquals(stringValues[i], ""); - } - break; - default: - throw new UnsupportedOperationException("Unsupported test type: " + resultsDataType); + @Test + public void testDistinctNullHandlingOffThrows() { + try { + givenCountryClickTable(false) + .whenQuery("SELECT DISTINCT jsonExtractIndex(flatJson, '$.country', 'STRING') FROM clicks") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected DISTINCT to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { + Assertions.assertThat(e.getMessage()).contains("Illegal Json Path"); } + } - RoaringBitmap nullBitmap = transformFunction.getNullBitmap(_projectionBlock); - Assert.assertNotNull(nullBitmap, "Null bitmap must be populated when all rows are unresolved"); - Assert.assertEquals(nullBitmap.getCardinality(), NUM_ROWS, - "Every row should be marked null when the JSON path doesn't resolve"); + /** Each row is `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */ + @DataProvider(name = "distinctCases") + public static Object[][] distinctCases() { + return new Object[][]{ + {"flatJson", "$.country", "STRING", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country"}, + {"flatJson", "$.clicks", "INT", new Object[][]{{3}, {5}, {null}}, "flatJson.clicks INT"}, + {"nestedJson", "$.location.country", "STRING", new Object[][]{{"US"}, {null}}, + "nestedJson.location.country"}, + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{{"green"}, {"red"}, {null}}, + "nestedJson.tags[0]"}, + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{{"US"}, {null}}, + "nestedJson.events[0].country"} + }; } - @Test(dataProvider = "unresolvedPathSvTypes") - public void testNullHandlingDisabledStillThrowsForUnresolvedPath(String resultsType, DataType resultsDataType) { - String expressionStr = String.format("jsonExtractIndex(%s,'$.noField','%s')", JSON_STRING_SV_COLUMN, resultsType); - ExpressionContext expression = RequestContextUtils.getExpression(expressionStr); - TransformFunction transformFunction = TransformFunctionFactory.get(expression, _dataSourceMap); + // ============================================================================================ + // 3. GROUP BY + // ============================================================================================ - Assert.assertTrue(transformFunction instanceof JsonExtractIndexTransformFunction); + /** + * GROUP BY with null handling ON. Unresolved rows must collapse into a single null group with + * the correct count, alongside the resolved value groups. + * 
+   *   Example — `flatJson.country` (STRING):
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
+   *
+   *     Query:  SET enableNullHandling = true;
+   *             SELECT jsonExtractIndex(flatJson, '$.country', 'STRING') AS v, COUNT(*)
+   *             FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST
+   *
+   *     Result (counts ×2 for segment duplication): ("CA", 2), ("US", 2), (NULL, 4)
+   *
+ */ + @Test(dataProvider = "groupByCases") + public void testGroupByNullHandlingOn(String column, String jsonPath, String resultsType, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractIndex(%s, '%s', '%s')", column, jsonPath, resultsType); + String query = String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr); + givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); + } + @Test + public void testGroupByNullHandlingOffThrows() { try { - switch (resultsDataType) { - case INT: - transformFunction.transformToIntValuesSV(_projectionBlock); - break; - case LONG: - transformFunction.transformToLongValuesSV(_projectionBlock); - break; - case FLOAT: - transformFunction.transformToFloatValuesSV(_projectionBlock); - break; - case DOUBLE: - transformFunction.transformToDoubleValuesSV(_projectionBlock); - break; - case BIG_DECIMAL: - transformFunction.transformToBigDecimalValuesSV(_projectionBlock); - break; - case STRING: - transformFunction.transformToStringValuesSV(_projectionBlock); - break; - default: - throw new UnsupportedOperationException("Unsupported test type: " + resultsDataType); - } - Assert.fail("Expected RuntimeException when null handling is disabled"); - } catch (RuntimeException e) { + givenCountryClickTable(false) + .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING'), COUNT(*) FROM clicks GROUP BY 1") + .thenResultIs(new Object[]{"unused"}); + Assert.fail("Expected GROUP BY to fail when null handling is off and rows are unresolved"); + } catch (AssertionError e) { Assertions.assertThat(e.getMessage()).contains("Illegal Json Path"); } } + + /** + * Each row is `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST, + * label)`. Counts are ×2 the per-row count due to segment duplication. + */ + @DataProvider(name = "groupByCases") + public static Object[][] groupByCases() { + return new Object[][]{ + {"flatJson", "$.country", "STRING", new Object[][]{ + {"CA", 2L}, {"US", 2L}, {null, 4L} + }, "flatJson.country"}, + {"flatJson", "$.clicks", "INT", new Object[][]{ + {3, 2L}, {5, 2L}, {null, 4L} + }, "flatJson.clicks INT"}, + {"nestedJson", "$.location.country", "STRING", new Object[][]{ + {"US", 2L}, {null, 6L} + }, "nestedJson.location.country"}, + {"nestedJson", "$.tags[0]", "STRING", new Object[][]{ + {"green", 2L}, {"red", 2L}, {null, 4L} + }, "nestedJson.tags[0]"}, + {"nestedJson", "$.events[0].country", "STRING", new Object[][]{ + {"US", 2L}, {null, 6L} + }, "nestedJson.events[0].country"} + }; + } } From 0b9790e7a0762e883c6485174a3eb965145ce81d Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Tue, 26 May 2026 13:30:03 -0700 Subject: [PATCH 06/14] Simplify: drop _emitNullOnUnresolved, use _nullHandlingEnabled directly In each SV transform, by the time control reaches the null-handling branch, the earlier `if (_defaultValue != null)` check has already filled defaults, so the remaining decision reduces to `_nullHandlingEnabled`. getNullBitmap inlines the equivalent short-circuit condition. No behavior change; 209 tests unchanged. --- .../JsonExtractIndexTransformFunction.java | 25 +++++++++-------- .../JsonExtractScalarTransformFunction.java | 27 ++++++++++--------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java index 96970b11bb..d5eb59ed63 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java @@ -55,11 +55,6 @@ public class JsonExtractIndexTransformFunction extends BaseTransformFunction { private Map _valueToMatchingDocsMap; private boolean _isSingleValue; private String _filterJsonPath; - // True when null-handling is enabled AND no default literal was supplied for the SV path. In that - // mode, single-value transforms emit the type's null placeholder for unresolved JSON paths (instead - // of throwing) and surface the unresolved row through {@link #getNullBitmap}, matching the broker's - // null-handling contract for upstream operators such as {@code JsonIndexDistinctOperator}. - private boolean _emitNullOnUnresolved; @Override public String getName() { @@ -154,7 +149,6 @@ public void init(List arguments, Map c _filterJsonPath = ((LiteralTransformFunction) fifthArgument).getStringLiteral(); } - _emitNullOnUnresolved = _isSingleValue && _nullHandlingEnabled && _defaultValue == null; _resultMetadata = new TransformResultMetadata(dataType, _isSingleValue, false); } @@ -177,7 +171,7 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) { _intValuesSV[i] = (int) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _intValuesSV[i] = NullValuePlaceHolder.INT; continue; } @@ -203,7 +197,7 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) { _longValuesSV[i] = (long) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _longValuesSV[i] = NullValuePlaceHolder.LONG; continue; } @@ -229,7 +223,7 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) { _floatValuesSV[i] = (float) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _floatValuesSV[i] = NullValuePlaceHolder.FLOAT; continue; } @@ -255,7 +249,7 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) { _doubleValuesSV[i] = (double) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _doubleValuesSV[i] = NullValuePlaceHolder.DOUBLE; continue; } @@ -281,7 +275,7 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) { _bigDecimalValuesSV[i] = (BigDecimal) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _bigDecimalValuesSV[i] = NullValuePlaceHolder.BIG_DECIMAL; continue; } @@ -307,7 +301,7 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { _stringValuesSV[i] = (String) _defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _stringValuesSV[i] = NullValuePlaceHolder.STRING; continue; } @@ -322,7 +316,12 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { @Override @Nullable public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { - if (!_emitNullOnUnresolved) { + // Short-circuit to argument-bitmap propagation when this function isn't introducing nulls of + // its own: non-SV output, null handling disabled, or any default literal supplied (the SV + // transform writes it for unresolved rows). Unlike the scalar function, the parser converts a + // SQL-NULL literal to the typed zero ("" for STRING, throws at init for numerics), so there's + // no SQL-NULL-default path to fall through. + if (!_isSingleValue || !_nullHandlingEnabled || _defaultValue != null) { return super.getNullBitmap(valueBlock); } String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(), diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java index d324fb2d4b..446a63a947 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java @@ -101,11 +101,6 @@ public class JsonExtractScalarTransformFunction extends BaseTransformFunction { private DataType _storedType; private Object _defaultValue; private boolean _defaultIsNull; - // True when null-handling is enabled AND no default literal was supplied. In that mode, single-value - // transforms emit the type's zero/empty sentinel for unresolved JSON paths (instead of throwing) and - // surface the unresolved row through {@link #getNullBitmap}, matching the broker's null-handling - // contract for upstream operators (e.g. JsonIndexDistinctOperator). - private boolean _emitNullOnUnresolved; private TransformResultMetadata _resultMetadata; @Override @@ -186,7 +181,6 @@ public void init(List arguments, Map c ); } } - _emitNullOnUnresolved = _nullHandlingEnabled && _defaultValue == null; _resultMetadata = new TransformResultMetadata(_dataType, isSingleValue, false); } @@ -198,7 +192,14 @@ public TransformResultMetadata getResultMetadata() { @Override @Nullable public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { - if (!_defaultIsNull && !_emitNullOnUnresolved) { + // Short-circuit to argument-bitmap propagation when this function isn't introducing nulls of + // its own. Two cases qualify: + // - null handling is disabled (the SV transform either fills a default or throws), or + // - a real (non-SQL-NULL) default was supplied (the SV transform writes the default for + // unresolved rows). + // The 4-arg SQL-NULL case has _defaultIsNull=true and falls through to the per-row scan + // below, so unresolved rows surface as SQL NULL via the bitmap. + if (!_nullHandlingEnabled || (_defaultValue != null && !_defaultIsNull)) { return super.getNullBitmap(valueBlock); } RoaringBitmap bitmap = new RoaringBitmap(); @@ -251,7 +252,7 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) { _intValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { // Write the null placeholder so stale data from a reused buffer can't surface. // getNullBitmap marks the row null; consumers should read the bitmap, not this value. _intValuesSV[i] = NullValuePlaceHolder.INT; @@ -286,7 +287,7 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) { _longValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _longValuesSV[i] = NullValuePlaceHolder.LONG; continue; } @@ -318,7 +319,7 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) { _floatValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _floatValuesSV[i] = NullValuePlaceHolder.FLOAT; continue; } @@ -350,7 +351,7 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) { _doubleValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _doubleValuesSV[i] = NullValuePlaceHolder.DOUBLE; continue; } @@ -382,7 +383,7 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) { _bigDecimalValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _bigDecimalValuesSV[i] = NullValuePlaceHolder.BIG_DECIMAL; continue; } @@ -414,7 +415,7 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) { _stringValuesSV[i] = defaultValue; continue; } - if (_emitNullOnUnresolved) { + if (_nullHandlingEnabled) { _stringValuesSV[i] = NullValuePlaceHolder.STRING; continue; } From da6062e1b75f6aa93a5f9db5e4193ca01e57c6b6 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Wed, 27 May 2026 14:16:18 -0700 Subject: [PATCH 07/14] Add default-precedence tests: NH on + non-null default returns default Three tests per file (projection, DISTINCT, GROUP BY) pin the SV transform's priority order: a user-supplied default wins over the null-handling placeholder. Without these, a future refactor could silently swap the order so unresolved rows surface as SQL NULL instead of the user's default. --- ...JsonExtractIndexTransformFunctionTest.java | 80 +++++++++++++++++++ ...sonExtractScalarTransformFunctionTest.java | 80 +++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java index e88b7e94f7..eb29bffcff 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java @@ -773,4 +773,84 @@ public static Object[][] groupByCases() { }, "nestedJson.events[0].country"} }; } + + // ============================================================================================ + // 4. Default-value precedence under null handling ON + // + // When a non-null default literal is supplied AND null handling is on, the SV transform's + // priority order is: real default > null-handling placeholder > throw. The user-supplied + // default surfaces for unresolved rows; the null-handling placeholder is NOT emitted, and + // no null bit is set in the bitmap. These tests pin that ordering across projection, + // DISTINCT, and GROUP BY so a future refactor can't silently swap the priority. + // ============================================================================================ + + /** + * Projection with NH on AND default `'foobar'`. Unresolved rows must surface as `"foobar"`, + * not SQL NULL. + * 
+   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *
+   *   Query: SET enableNullHandling = true;
+   *          SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c
+   *          FROM clicks ORDER BY c ASC
+   *
+   *   Result rows (×2 for segment duplication; ASCII order: uppercase < lowercase):
+   *           "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
+   *
+ */ + @Test + public void testProjectionDefaultBeatsNullHandlingPlaceholder() { + givenCountryClickTable(true) + .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c " + + "FROM clicks ORDER BY c ASC") + .thenResultIs( + new Object[]{"CA"}, new Object[]{"CA"}, + new Object[]{"US"}, new Object[]{"US"}, + new Object[]{"foobar"}, new Object[]{"foobar"}, + new Object[]{"foobar"}, new Object[]{"foobar"}); + } + + /** + * DISTINCT with NH on AND default `'foobar'`. The distinct set must include the default value + * as a regular distinct entry — no separate null entry. + * 
+   *   Query: SET enableNullHandling = true;
+   *          SELECT DISTINCT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') FROM clicks
+   *          ORDER BY jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') ASC
+   *
+   *   Result (ASCII order: uppercase < lowercase): "CA", "US", "foobar"
+   *
+ */ + @Test + public void testDistinctDefaultBeatsNullHandlingPlaceholder() { + String expr = "jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar')"; + givenCountryClickTable(true) + .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) + .thenResultIs(new Object[]{"CA"}, new Object[]{"US"}, new Object[]{"foobar"}); + } + + /** + * GROUP BY with NH on AND default `'foobar'`. Unresolved rows count toward the default's + * group, not a null group — so the result has no NULL group at all. + * 
+   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *
+   *   Query: SET enableNullHandling = true;
+   *          SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*)
+   *          FROM clicks GROUP BY v ORDER BY v ASC
+   *
+   *   Result (counts ×2 for segment duplication; ASCII order: uppercase < lowercase):
+   *           ("CA", 2), ("US", 2), ("foobar", 4)
+   *
+ */ + @Test + public void testGroupByDefaultBeatsNullHandlingPlaceholder() { + givenCountryClickTable(true) + .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*) " + + "FROM clicks GROUP BY v ORDER BY v ASC") + .thenResultIs( + new Object[]{"CA", 2L}, + new Object[]{"US", 2L}, + new Object[]{"foobar", 4L}); + } } diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index bf3dc0b907..c5cb1d10a1 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -1208,4 +1208,84 @@ public static Object[][] groupByCases() { }, "nestedJson.events[0].country"} }; } + + // ============================================================================================ + // 4. Default-value precedence under null handling ON + // + // When a non-null default literal is supplied AND null handling is on, the SV transform's + // priority order is: real default > null-handling placeholder > throw. The user-supplied + // default surfaces for unresolved rows; the null-handling placeholder is NOT emitted, and + // no null bit is set in the bitmap. These tests pin that ordering across projection, + // DISTINCT, and GROUP BY so a future refactor can't silently swap the priority. + // ============================================================================================ + + /** + * Projection with NH on AND default `'foobar'`. Unresolved rows must surface as `"foobar"`, + * not SQL NULL. + * 
+   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *
+   *   Query: SET enableNullHandling = true;
+   *          SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c
+   *          FROM clicks ORDER BY c ASC
+   *
+   *   Result rows (×2 for segment duplication; ASCII order: uppercase < lowercase):
+   *           "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
+   *
+ */ + @Test + public void testProjectionDefaultBeatsNullHandlingPlaceholder() { + givenCountryClickTable(true) + .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c " + + "FROM clicks ORDER BY c ASC") + .thenResultIs( + new Object[]{"CA"}, new Object[]{"CA"}, + new Object[]{"US"}, new Object[]{"US"}, + new Object[]{"foobar"}, new Object[]{"foobar"}, + new Object[]{"foobar"}, new Object[]{"foobar"}); + } + + /** + * DISTINCT with NH on AND default `'foobar'`. The distinct set must include the default value + * as a regular distinct entry — no separate null entry. + * 
+   *   Query: SET enableNullHandling = true;
+   *          SELECT DISTINCT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') FROM clicks
+   *          ORDER BY jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') ASC
+   *
+   *   Result (ASCII order: uppercase < lowercase): "CA", "US", "foobar"
+   *
+ */ + @Test + public void testDistinctDefaultBeatsNullHandlingPlaceholder() { + String expr = "jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar')"; + givenCountryClickTable(true) + .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) + .thenResultIs(new Object[]{"CA"}, new Object[]{"US"}, new Object[]{"foobar"}); + } + + /** + * GROUP BY with NH on AND default `'foobar'`. Unresolved rows count toward the default's + * group, not a null group — so the result has no NULL group at all. + * 
+   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *
+   *   Query: SET enableNullHandling = true;
+   *          SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*)
+   *          FROM clicks GROUP BY v ORDER BY v ASC
+   *
+   *   Result (counts ×2 for segment duplication; ASCII order: uppercase < lowercase):
+   *           ("CA", 2), ("US", 2), ("foobar", 4)
+   *
+ */ + @Test + public void testGroupByDefaultBeatsNullHandlingPlaceholder() { + givenCountryClickTable(true) + .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*) " + + "FROM clicks GROUP BY v ORDER BY v ASC") + .thenResultIs( + new Object[]{"CA", 2L}, + new Object[]{"US", 2L}, + new Object[]{"foobar", 4L}); + } } From f0d7f6b66afec3bcaeb714f2c22bb236035bcf92 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Wed, 27 May 2026 14:36:30 -0700 Subject: [PATCH 08/14] Close coverage gaps: nestedJson + SQL-NULL-literal default cases Default-precedence tests in both files now parameterize over flatJson + nestedJson (mirrors the projection/DISTINCT/GROUP BY data providers). Scalar test adds Section 5 covering 4-arg with SQL NULL literal under NH on, pinning the _defaultIsNull code path in getNullBitmap. --- ...JsonExtractIndexTransformFunctionTest.java | 131 +++++++---- ...sonExtractScalarTransformFunctionTest.java | 213 ++++++++++++++---- 2 files changed, 247 insertions(+), 97 deletions(-) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java index eb29bffcff..79455eaab8 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java @@ -785,72 +785,103 @@ public static Object[][] groupByCases() { // ============================================================================================ /** - * Projection with NH on AND default `'foobar'`. Unresolved rows must surface as `"foobar"`, - * not SQL NULL. + * Projection with NH on AND a non-null default literal. Unresolved rows must surface as the + * default, not SQL NULL — across both flat scalar and nested-object shapes. * 
-   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *   Example — flatJson.country with default 'foobar':
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar" (was null),
+   *              row 3 -> "foobar" (was missing)
    *
-   *   Query: SET enableNullHandling = true;
-   *          SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c
-   *          FROM clicks ORDER BY c ASC
+   *     Query: SET enableNullHandling = true;
+   *            SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c
+   *            FROM clicks ORDER BY c ASC
    *
-   *   Result rows (×2 for segment duplication; ASCII order: uppercase < lowercase):
-   *           "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
+   *     Result rows (×2 segment dup; ASCII order, uppercase before lowercase):
+   *             "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
    *
*/ - @Test - public void testProjectionDefaultBeatsNullHandlingPlaceholder() { + @Test(dataProvider = "defaultPrecedenceProjectionCases") + public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); givenCountryClickTable(true) - .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c " - + "FROM clicks ORDER BY c ASC") - .thenResultIs( - new Object[]{"CA"}, new Object[]{"CA"}, - new Object[]{"US"}, new Object[]{"US"}, - new Object[]{"foobar"}, new Object[]{"foobar"}, - new Object[]{"foobar"}, new Object[]{"foobar"}); + .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr)) + .thenResultIs(expectedRows); } /** - * DISTINCT with NH on AND default `'foobar'`. The distinct set must include the default value - * as a regular distinct entry — no separate null entry. - * 
-   *   Query: SET enableNullHandling = true;
-   *          SELECT DISTINCT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') FROM clicks
-   *          ORDER BY jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') ASC
-   *
-   *   Result (ASCII order: uppercase < lowercase): "CA", "US", "foobar"
-   *
+ * Each row: `(column, jsonPath, default SQL literal, expected 8-row result, label)`. Result + * rows are ordered ASC. Each per-row value in the fixture appears ×2 due to segment duplication. */ - @Test - public void testDistinctDefaultBeatsNullHandlingPlaceholder() { - String expr = "jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar')"; + @DataProvider(name = "defaultPrecedenceProjectionCases") + public static Object[][] defaultPrecedenceProjectionCases() { + return new Object[][]{ + // flatJson.country: row0="US", row1="CA", row2=null, row3=missing -> 2 each, 4 'foobar'. + {"flatJson", "$.country", "'foobar'", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "flatJson.country default 'foobar'"}, + // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 'foobar'. + {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ + {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "nestedJson.location.country default 'foobar'"} + }; + } + + /** + * DISTINCT with NH on AND a non-null default literal. The distinct set includes the default + * value as a regular entry — no separate null entry. + */ + @Test(dataProvider = "defaultPrecedenceDistinctCases") + public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); givenCountryClickTable(true) .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) - .thenResultIs(new Object[]{"CA"}, new Object[]{"US"}, new Object[]{"foobar"}); + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceDistinctCases") + public static Object[][] defaultPrecedenceDistinctCases() { + return new Object[][]{ + {"flatJson", "$.country", "'foobar'", + new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"}, + {"nestedJson", "$.location.country", "'foobar'", + new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"} + }; } /** - * GROUP BY with NH on AND default `'foobar'`. Unresolved rows count toward the default's - * group, not a null group — so the result has no NULL group at all. - * 
-   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
-   *
-   *   Query: SET enableNullHandling = true;
-   *          SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*)
-   *          FROM clicks GROUP BY v ORDER BY v ASC
-   *
-   *   Result (counts ×2 for segment duplication; ASCII order: uppercase < lowercase):
-   *           ("CA", 2), ("US", 2), ("foobar", 4)
-   *
+ * GROUP BY with NH on AND a non-null default literal. Unresolved rows count toward the + * default's group — no NULL group surfaces. */ - @Test - public void testGroupByDefaultBeatsNullHandlingPlaceholder() { + @Test(dataProvider = "defaultPrecedenceGroupByCases") + public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); givenCountryClickTable(true) - .whenQuery("SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*) " - + "FROM clicks GROUP BY v ORDER BY v ASC") - .thenResultIs( - new Object[]{"CA", 2L}, - new Object[]{"US", 2L}, - new Object[]{"foobar", 4L}); + .whenQuery(String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr)) + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceGroupByCases") + public static Object[][] defaultPrecedenceGroupByCases() { + return new Object[][]{ + // flatJson.country: 2 "CA" + 2 "US" + 4 'foobar'. + {"flatJson", "$.country", "'foobar'", new Object[][]{ + {"CA", 2L}, {"US", 2L}, {"foobar", 4L} + }, "flatJson.country default 'foobar'"}, + // nestedJson.location.country: 2 "US" + 6 'foobar'. + {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ + {"US", 2L}, {"foobar", 6L} + }, "nestedJson.location.country default 'foobar'"} + }; } } diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index c5cb1d10a1..fa35410a3f 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -1220,72 +1220,191 @@ public static Object[][] groupByCases() { // ============================================================================================ /** - * Projection with NH on AND default `'foobar'`. Unresolved rows must surface as `"foobar"`, - * not SQL NULL. + * Projection with NH on AND a non-null default literal. Unresolved rows must surface as the + * default, not SQL NULL — across both flat scalar and nested-object shapes. * 
-   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *   Example — flatJson.country with default 'foobar':
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar" (was null),
+   *              row 3 -> "foobar" (was missing)
    *
-   *   Query: SET enableNullHandling = true;
-   *          SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c
-   *          FROM clicks ORDER BY c ASC
+   *     Query: SET enableNullHandling = true;
+   *            SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c
+   *            FROM clicks ORDER BY c ASC
    *
-   *   Result rows (×2 for segment duplication; ASCII order: uppercase < lowercase):
-   *           "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
+   *     Result rows (×2 segment dup; ASCII order, uppercase before lowercase):
+   *             "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
    *
*/ - @Test - public void testProjectionDefaultBeatsNullHandlingPlaceholder() { + @Test(dataProvider = "defaultPrecedenceProjectionCases") + public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); givenCountryClickTable(true) - .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c " - + "FROM clicks ORDER BY c ASC") - .thenResultIs( - new Object[]{"CA"}, new Object[]{"CA"}, - new Object[]{"US"}, new Object[]{"US"}, - new Object[]{"foobar"}, new Object[]{"foobar"}, - new Object[]{"foobar"}, new Object[]{"foobar"}); + .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr)) + .thenResultIs(expectedRows); } /** - * DISTINCT with NH on AND default `'foobar'`. The distinct set must include the default value - * as a regular distinct entry — no separate null entry. - * 
-   *   Query: SET enableNullHandling = true;
-   *          SELECT DISTINCT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') FROM clicks
-   *          ORDER BY jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') ASC
-   *
-   *   Result (ASCII order: uppercase < lowercase): "CA", "US", "foobar"
-   *
+ * Each row: `(column, jsonPath, default SQL literal, expected 8-row result, label)`. Result + * rows are ordered ASC. Each per-row value in the fixture appears ×2 due to segment duplication. */ - @Test - public void testDistinctDefaultBeatsNullHandlingPlaceholder() { - String expr = "jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar')"; + @DataProvider(name = "defaultPrecedenceProjectionCases") + public static Object[][] defaultPrecedenceProjectionCases() { + return new Object[][]{ + // flatJson.country: row0="US", row1="CA", row2=null, row3=missing -> 2 each, 4 'foobar'. + {"flatJson", "$.country", "'foobar'", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "flatJson.country default 'foobar'"}, + // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 'foobar'. + {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ + {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "nestedJson.location.country default 'foobar'"} + }; + } + + /** + * DISTINCT with NH on AND a non-null default literal. The distinct set includes the default + * value as a regular entry — no separate null entry. + */ + @Test(dataProvider = "defaultPrecedenceDistinctCases") + public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); givenCountryClickTable(true) .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) - .thenResultIs(new Object[]{"CA"}, new Object[]{"US"}, new Object[]{"foobar"}); + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceDistinctCases") + public static Object[][] defaultPrecedenceDistinctCases() { + return new Object[][]{ + {"flatJson", "$.country", "'foobar'", + new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"}, + {"nestedJson", "$.location.country", "'foobar'", + new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"} + }; + } + + /** + * GROUP BY with NH on AND a non-null default literal. Unresolved rows count toward the + * default's group — no NULL group surfaces. + */ + @Test(dataProvider = "defaultPrecedenceGroupByCases") + public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); + givenCountryClickTable(true) + .whenQuery(String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr)) + .thenResultIs(expectedRows); } + /** Each row: `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceGroupByCases") + public static Object[][] defaultPrecedenceGroupByCases() { + return new Object[][]{ + // flatJson.country: 2 "CA" + 2 "US" + 4 'foobar'. + {"flatJson", "$.country", "'foobar'", new Object[][]{ + {"CA", 2L}, {"US", 2L}, {"foobar", 4L} + }, "flatJson.country default 'foobar'"}, + // nestedJson.location.country: 2 "US" + 6 'foobar'. + {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ + {"US", 2L}, {"foobar", 6L} + }, "nestedJson.location.country default 'foobar'"} + }; + } + + // ============================================================================================ + // 5. 4-arg with SQL NULL literal as default, under null handling ON + // + // Result is identical to the 3-arg + NH-on case (unresolved rows surface as SQL NULL), but + // the code path is different: `init` sets `_defaultValue` to the typed zero (0, "", etc.) and + // marks `_defaultIsNull=true`. The SV transform writes the typed zero via the + // `_defaultValue != null` branch; `getNullBitmap` scans and marks unresolved rows null thanks + // to the `!_defaultIsNull` carve-out in its short-circuit condition. These tests pin that + // code path so a future refactor that removes `_defaultIsNull` won't silently break this case. + // ============================================================================================ + /** - * GROUP BY with NH on AND default `'foobar'`. Unresolved rows count toward the default's - * group, not a null group — so the result has no NULL group at all. + * Projection with NH on AND `NULL` literal as the default. Unresolved rows surface as SQL NULL, + * same as the 3-arg form. * 
-   *   Per-row resolution: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
-   *
-   *   Query: SET enableNullHandling = true;
-   *          SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*)
-   *          FROM clicks GROUP BY v ORDER BY v ASC
+   *   Example — flatJson.country:
+   *     Query: SET enableNullHandling = true;
+   *            SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', NULL) AS c
+   *            FROM clicks ORDER BY c ASC NULLS LAST
    *
-   *   Result (counts ×2 for segment duplication; ASCII order: uppercase < lowercase):
-   *           ("CA", 2), ("US", 2), ("foobar", 4)
+   *     Result rows (×2 segment dup):
+   *             "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
    *
*/ - @Test - public void testGroupByDefaultBeatsNullHandlingPlaceholder() { + @Test(dataProvider = "sqlNullDefaultProjectionCases") + public void testProjectionWithSqlNullDefaultEmitsNull(String column, String jsonPath, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath); + givenCountryClickTable(true) + .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC NULLS LAST", expr)) + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, expected 8-row result ordered ASC NULLS LAST, label)`. */ + @DataProvider(name = "sqlNullDefaultProjectionCases") + public static Object[][] sqlNullDefaultProjectionCases() { + return new Object[][]{ + {"flatJson", "$.country", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null} + }, "flatJson.country NULL default"}, + {"nestedJson", "$.location.country", new Object[][]{ + {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} + }, "nestedJson.location.country NULL default"} + }; + } + + /** DISTINCT with NH on AND `NULL` literal as default — the distinct set includes a null entry. */ + @Test(dataProvider = "sqlNullDefaultDistinctCases") + public void testDistinctWithSqlNullDefaultIncludesNull(String column, String jsonPath, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath); givenCountryClickTable(true) - .whenQuery("SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS v, COUNT(*) " - + "FROM clicks GROUP BY v ORDER BY v ASC") - .thenResultIs( - new Object[]{"CA", 2L}, - new Object[]{"US", 2L}, - new Object[]{"foobar", 4L}); + .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr)) + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, expected distinct rows ASC NULLS LAST, label)`. */ + @DataProvider(name = "sqlNullDefaultDistinctCases") + public static Object[][] sqlNullDefaultDistinctCases() { + return new Object[][]{ + {"flatJson", "$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country NULL default"}, + {"nestedJson", "$.location.country", new Object[][]{{"US"}, {null}}, + "nestedJson.location.country NULL default"} + }; + } + + /** GROUP BY with NH on AND `NULL` literal as default — unresolved rows form a null group. */ + @Test(dataProvider = "sqlNullDefaultGroupByCases") + public void testGroupByWithSqlNullDefaultProducesNullGroup(String column, String jsonPath, + Object[][] expectedRows, String label) { + String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath); + givenCountryClickTable(true) + .whenQuery(String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr)) + .thenResultIs(expectedRows); + } + + /** Each row: `(column, jsonPath, expected (value, count) rows ASC NULLS LAST, label)`. */ + @DataProvider(name = "sqlNullDefaultGroupByCases") + public static Object[][] sqlNullDefaultGroupByCases() { + return new Object[][]{ + {"flatJson", "$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}}, + "flatJson.country NULL default"}, + {"nestedJson", "$.location.country", new Object[][]{{"US", 2L}, {null, 6L}}, + "nestedJson.location.country NULL default"} + }; } } From 3bcb5d160c7ae246612f08fe3119000c76317b80 Mon Sep 17 00:00:00 2001 From: Siddharth Teotia Date: Wed, 27 May 2026 15:01:52 -0700 Subject: [PATCH 09/14] Reorganize country/click tests: group all cases by query shape Both test files now have Section 1 (Projection), 2 (DISTINCT), 3 (GROUP BY) where each section owns its own sub-cases (.1 NH-on 3-arg, .2 NH-on with SQL-NULL default [scalar only], .3 NH-on with non-null default, .4 NH-off). Previously, default-precedence and SQL-NULL-default tests lived in separate sections 4/5 even though they belonged to the same three query shapes. --- ...JsonExtractIndexTransformFunctionTest.java | 262 +++++------- ...sonExtractScalarTransformFunctionTest.java | 399 ++++++++---------- 2 files changed, 279 insertions(+), 382 deletions(-) diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java index 79455eaab8..d917a17259 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java @@ -456,11 +456,18 @@ private T getValueForKey(String blob, JsonPath path, TypeRef typeRef) { // `nestedJson` — multi-level shapes (nested object, array, array of objects) with mixed // resolved / null / missing data at depth. // - // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each group - // has a null-handling-on case (verifies SQL NULL surfaces) and a null-handling-off case - // (verifies the legacy throw is preserved). Result rows are ordered by the projected expression - // ASC NULLS LAST so the comparison is deterministic across the framework's segment-duplication - // behavior (one segment becomes two, so per-row counts in GROUP BY results are ×2). + // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each section + // covers three sub-cases, in order: + // + // .1 NH on, 3-arg — unresolved rows surface as SQL NULL (E4 fix) + // .2 NH on, 4-arg with a non-null default — default wins over NH placeholder + // .3 NH off — legacy throw preserved + // + // Unlike the scalar function, the index function has no `_defaultIsNull` path: a `NULL` literal + // as the 4-arg default fails at init (numeric) or silently becomes empty string (STRING) — both + // pre-existing quirks outside the scope of this PR. Result rows in every assertion are ordered + // by the projected expression ASC NULLS LAST so the comparison is deterministic across the + // framework's segment-duplication behavior (one segment becomes two, so per-row counts ×2). // ============================================================================================ protected File _baseDir; @@ -562,18 +569,15 @@ private FluentQueryTest.OnFirstInstance givenCountryClickTable(boolean nullHandl // ============================================================================================ /** - * Projection with null handling ON. Each row's resolution depends on the column + path; the SV - * transform must surface SQL NULL for unresolved rows and pass resolved values through. + * 1.1 — NH on, 3-arg. The SV transform must surface SQL NULL for unresolved rows and pass + * resolved values through. * 
    *   Example — `flatJson.country` (STRING):
    *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
-   *
    *     Query:  SET enableNullHandling = true;
    *             SELECT jsonExtractIndex(flatJson, '$.country', 'STRING') AS c
    *             FROM clicks ORDER BY c ASC NULLS LAST
-   *
-   *     Result rows (×2 for segment duplication):
-   *             "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
+   *     Result (×2 dup): "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
    *
*/ @Test(dataProvider = "projectionCases") @@ -586,10 +590,24 @@ public void testProjectionNullHandlingOn(String column, String jsonPath, String } /** - * Null handling OFF: any unresolved row throws (legacy behavior, preserved by the fix's - * `_nullHandlingEnabled` gate). One representative path is enough — the throw is independent of - * path shape and result type. The error message for `jsonExtractIndex` differs from the scalar - * function's: `"Illegal Json Path: [...], for docId [...]"`. + * 1.2 — NH on, 4-arg with non-null default. The SV transform's priority is: real default > + * null-handling placeholder > throw. The user-supplied default surfaces for unresolved rows; + * no null bit is set in the bitmap. + */ + @Test(dataProvider = "defaultPrecedenceProjectionCases") + public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); + givenCountryClickTable(true) + .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr)) + .thenResultIs(expectedRows); + } + + /** + * 1.3 — NH off: any unresolved row throws. One representative path; throw is path-independent. + * The index function's error message differs from the scalar's ("Illegal Json Path" vs "Cannot + * resolve JSON path"). */ @Test public void testProjectionNullHandlingOffThrows() { @@ -603,11 +621,7 @@ public void testProjectionNullHandlingOffThrows() { } } - /** - * Each row is `(column, jsonPath, resultsType, expected 8-row result, label)`. Result rows are - * ordered by value ASC NULLS LAST. With segment duplication, the per-row count in the fixture - * is ×2 in the result. - */ + /** 1.1 cases — `(column, jsonPath, resultsType, expected 8-row result, label)`, ordered ASC NULLS LAST. */ @DataProvider(name = "projectionCases") public static Object[][] projectionCases() { return new Object[][]{ @@ -629,51 +643,52 @@ public static Object[][] projectionCases() { {"flatJson", "$.clicks", "DOUBLE", new Object[][]{ {3d}, {3d}, {5d}, {5d}, {null}, {null}, {null}, {null} }, "flatJson.clicks DOUBLE"}, - // BIG_DECIMAL is formatted as String by the broker (BigDecimal.toPlainString). + // BIG_DECIMAL is formatted as String by the broker. {"flatJson", "$.clicks", "BIG_DECIMAL", new Object[][]{ {"3"}, {"3"}, {"5"}, {"5"}, {null}, {null}, {null}, {null} }, "flatJson.clicks BIG_DECIMAL"}, // ----- nestedJson: multi-level shapes ----- - // $.location.country: row0="US", row1=missing (no country key), row2=parent null, row3=missing. {"nestedJson", "$.location.country", "STRING", new Object[][]{ {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} }, "nestedJson.location.country STRING"}, - // $.tags[0]: row0="red", row1="green", row2=null parent, row3=missing. {"nestedJson", "$.tags[0]", "STRING", new Object[][]{ {"green"}, {"green"}, {"red"}, {"red"}, {null}, {null}, {null}, {null} }, "nestedJson.tags[0] STRING"}, - // $.tags[1]: row0="blue", row1=out-of-bounds (only one element), row2=null parent, row3=missing. {"nestedJson", "$.tags[1]", "STRING", new Object[][]{ {"blue"}, {"blue"}, {null}, {null}, {null}, {null}, {null}, {null} }, "nestedJson.tags[1] STRING (OOB on row 1)"}, - // $.events[0].country: row0="US", row1=explicit inner null, row2=null array, row3=missing. {"nestedJson", "$.events[0].country", "STRING", new Object[][]{ {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null} }, "nestedJson.events[0].country STRING"} }; } + /** 1.2 cases — `(column, jsonPath, default SQL literal, expected 8-row result ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceProjectionCases") + public static Object[][] defaultPrecedenceProjectionCases() { + return new Object[][]{ + {"flatJson", "$.country", "'foobar'", new Object[][]{ + {"CA"}, {"CA"}, {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "flatJson.country default 'foobar'"}, + {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ + {"US"}, {"US"}, + {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} + }, "nestedJson.location.country default 'foobar'"} + }; + } + // ============================================================================================ // 2. DISTINCT + // + // Pinot rejects positional `ORDER BY 1` for DISTINCT, so each DISTINCT test repeats the + // projected expression in ORDER BY. // ============================================================================================ /** - * DISTINCT with null handling ON. The distinct set must include exactly one null entry - * alongside the resolved values (deduped across segments). - * 
-   *   Example — `flatJson.country` (STRING):
-   *     Query:  SET enableNullHandling = true;
-   *             SELECT DISTINCT jsonExtractIndex(flatJson, '$.country', 'STRING') FROM clicks
-   *             ORDER BY jsonExtractIndex(flatJson, '$.country', 'STRING') ASC NULLS LAST
-   *
-   *     Result: "CA", "US", NULL
-   *
- * For STRING DISTINCT against a JSON-indexed column, the broker actually routes the query - * through {@code JsonIndexDistinctOperator} (not the transform-fn path) — so this case - * indirectly exercises that operator. The other types fall back to the transform-fn path - * because the operator's parser converts the default literal in a way that fails for - * non-string types; that fallback is what this fix corrects. + * 2.1 — NH on, 3-arg. The distinct set must include exactly one null entry alongside the + * resolved values (deduped across segments). */ @Test(dataProvider = "distinctCases") public void testDistinctNullHandlingOn(String column, String jsonPath, String resultsType, @@ -683,6 +698,18 @@ public void testDistinctNullHandlingOn(String column, String jsonPath, String re givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); } + /** 2.2 — NH on, 4-arg with non-null default. Default appears as a regular distinct entry (no null). */ + @Test(dataProvider = "defaultPrecedenceDistinctCases") + public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); + givenCountryClickTable(true) + .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) + .thenResultIs(expectedRows); + } + + /** 2.3 — NH off: DISTINCT throws on unresolved rows. */ @Test public void testDistinctNullHandlingOffThrows() { try { @@ -695,7 +722,7 @@ public void testDistinctNullHandlingOffThrows() { } } - /** Each row is `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */ + /** 2.1 cases — `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */ @DataProvider(name = "distinctCases") public static Object[][] distinctCases() { return new Object[][]{ @@ -710,33 +737,51 @@ public static Object[][] distinctCases() { }; } + /** 2.2 cases — `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */ + @DataProvider(name = "defaultPrecedenceDistinctCases") + public static Object[][] defaultPrecedenceDistinctCases() { + return new Object[][]{ + {"flatJson", "$.country", "'foobar'", + new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"}, + {"nestedJson", "$.location.country", "'foobar'", + new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"} + }; + } + // ============================================================================================ // 3. GROUP BY // ============================================================================================ /** - * GROUP BY with null handling ON. Unresolved rows must collapse into a single null group with - * the correct count, alongside the resolved value groups. - * 
-   *   Example — `flatJson.country` (STRING):
-   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
-   *
-   *     Query:  SET enableNullHandling = true;
-   *             SELECT jsonExtractIndex(flatJson, '$.country', 'STRING') AS v, COUNT(*)
-   *             FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST
-   *
-   *     Result (counts ×2 for segment duplication): ("CA", 2), ("US", 2), (NULL, 4)
-   *
+ * 3.1 — NH on, 3-arg. Unresolved rows collapse into a single null group with the correct count, + * alongside the resolved value groups. */ @Test(dataProvider = "groupByCases") public void testGroupByNullHandlingOn(String column, String jsonPath, String resultsType, Object[][] expectedRows, String label) { String expr = String.format("jsonExtractIndex(%s, '%s', '%s')", column, jsonPath, resultsType); - String query = String.format( - "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr); - givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows); + givenCountryClickTable(true) + .whenQuery(String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr)) + .thenResultIs(expectedRows); + } + + /** + * 3.2 — NH on, 4-arg with non-null default. Unresolved rows count toward the default's group; + * no NULL group surfaces. + */ + @Test(dataProvider = "defaultPrecedenceGroupByCases") + public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, + String defaultLiteral, Object[][] expectedRows, String label) { + String expr = String.format( + "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); + givenCountryClickTable(true) + .whenQuery(String.format( + "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr)) + .thenResultIs(expectedRows); } + /** 3.3 — NH off: GROUP BY throws on unresolved rows. */ @Test public void testGroupByNullHandlingOffThrows() { try { @@ -749,10 +794,7 @@ public void testGroupByNullHandlingOffThrows() { } } - /** - * Each row is `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST, - * label)`. Counts are ×2 the per-row count due to segment duplication. - */ + /** 3.1 cases — `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST, label)`. */ @DataProvider(name = "groupByCases") public static Object[][] groupByCases() { return new Object[][]{ @@ -774,111 +816,13 @@ public static Object[][] groupByCases() { }; } - // ============================================================================================ - // 4. Default-value precedence under null handling ON - // - // When a non-null default literal is supplied AND null handling is on, the SV transform's - // priority order is: real default > null-handling placeholder > throw. The user-supplied - // default surfaces for unresolved rows; the null-handling placeholder is NOT emitted, and - // no null bit is set in the bitmap. These tests pin that ordering across projection, - // DISTINCT, and GROUP BY so a future refactor can't silently swap the priority. - // ============================================================================================ - - /** - * Projection with NH on AND a non-null default literal. Unresolved rows must surface as the - * default, not SQL NULL — across both flat scalar and nested-object shapes. - * 
-   *   Example — flatJson.country with default 'foobar':
-   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar" (was null),
-   *              row 3 -> "foobar" (was missing)
-   *
-   *     Query: SET enableNullHandling = true;
-   *            SELECT jsonExtractIndex(flatJson, '$.country', 'STRING', 'foobar') AS c
-   *            FROM clicks ORDER BY c ASC
-   *
-   *     Result rows (×2 segment dup; ASCII order, uppercase before lowercase):
-   *             "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
-   *
- */ - @Test(dataProvider = "defaultPrecedenceProjectionCases") - public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, - String defaultLiteral, Object[][] expectedRows, String label) { - String expr = String.format( - "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); - givenCountryClickTable(true) - .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr)) - .thenResultIs(expectedRows); - } - - /** - * Each row: `(column, jsonPath, default SQL literal, expected 8-row result, label)`. Result - * rows are ordered ASC. Each per-row value in the fixture appears ×2 due to segment duplication. - */ - @DataProvider(name = "defaultPrecedenceProjectionCases") - public static Object[][] defaultPrecedenceProjectionCases() { - return new Object[][]{ - // flatJson.country: row0="US", row1="CA", row2=null, row3=missing -> 2 each, 4 'foobar'. - {"flatJson", "$.country", "'foobar'", new Object[][]{ - {"CA"}, {"CA"}, {"US"}, {"US"}, - {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} - }, "flatJson.country default 'foobar'"}, - // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 'foobar'. - {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ - {"US"}, {"US"}, - {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"} - }, "nestedJson.location.country default 'foobar'"} - }; - } - - /** - * DISTINCT with NH on AND a non-null default literal. The distinct set includes the default - * value as a regular entry — no separate null entry. - */ - @Test(dataProvider = "defaultPrecedenceDistinctCases") - public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, - String defaultLiteral, Object[][] expectedRows, String label) { - String expr = String.format( - "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); - givenCountryClickTable(true) - .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr)) - .thenResultIs(expectedRows); - } - - /** Each row: `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */ - @DataProvider(name = "defaultPrecedenceDistinctCases") - public static Object[][] defaultPrecedenceDistinctCases() { - return new Object[][]{ - {"flatJson", "$.country", "'foobar'", - new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"}, - {"nestedJson", "$.location.country", "'foobar'", - new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"} - }; - } - - /** - * GROUP BY with NH on AND a non-null default literal. Unresolved rows count toward the - * default's group — no NULL group surfaces. - */ - @Test(dataProvider = "defaultPrecedenceGroupByCases") - public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath, - String defaultLiteral, Object[][] expectedRows, String label) { - String expr = String.format( - "jsonExtractIndex(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral); - givenCountryClickTable(true) - .whenQuery(String.format( - "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr)) - .thenResultIs(expectedRows); - } - - /** Each row: `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */ + /** 3.2 cases — `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */ @DataProvider(name = "defaultPrecedenceGroupByCases") public static Object[][] defaultPrecedenceGroupByCases() { return new Object[][]{ - // flatJson.country: 2 "CA" + 2 "US" + 4 'foobar'. {"flatJson", "$.country", "'foobar'", new Object[][]{ {"CA", 2L}, {"US", 2L}, {"foobar", 4L} }, "flatJson.country default 'foobar'"}, - // nestedJson.location.country: 2 "US" + 6 'foobar'. {"nestedJson", "$.location.country", "'foobar'", new Object[][]{ {"US", 2L}, {"foobar", 6L} }, "nestedJson.location.country default 'foobar'"} diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java index fa35410a3f..e49f7f6eaa 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java @@ -918,11 +918,18 @@ public void testCrossTypeConversionFromStringResult() { // `nestedJson` — multi-level shapes (nested object, array, array of objects) with mixed // resolved / null / missing data at depth. // - // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each group - // has a null-handling-on case (verifies SQL NULL surfaces) and a null-handling-off case - // (verifies the legacy throw is preserved). Result rows in every assertion are ordered by the - // projected expression ASC NULLS LAST so the comparison is deterministic across the framework's - // segment-duplication behavior (a single segment shows up twice, so per-row counts are ×2). + // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each section + // covers the same four sub-cases, in order: + // + // .1 NH on, 3-arg — unresolved rows surface as SQL NULL (E4 fix) + // .2 NH on, 4-arg with SQL NULL literal — same result as .1 via the `_defaultIsNull` + // code path in `getNullBitmap` + // .3 NH on, 4-arg with a non-null default — default wins over NH placeholder + // .4 NH off — legacy throw preserved + // + // Result rows in every assertion are ordered by the projected expression ASC NULLS LAST so the + // comparison is deterministic across the framework's segment-duplication behavior (a single + // segment shows up twice, so per-row counts are ×2). // ============================================================================================ // ---------------- Fixture ---------------- @@ -995,8 +1002,8 @@ private FluentQueryTest.OnFirstInstance givenCountryClickTable(boolean nullHandl // ============================================================================================ /** - * Projection with null handling ON. Each row's resolution depends on the column + path; the SV - * transform must surface SQL NULL for unresolved rows and pass resolved values through. + * 1.1 — NH on, 3-arg. Each row's resolution depends on the column + path; the SV transform must + * surface SQL NULL for unresolved rows and pass resolved values through. * 
    *   Example — `flatJson.country` (STRING):
    *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
@@ -1019,9 +1026,45 @@ public void testProjectionNullHandlingOn(String column, String jsonPath, String
   }
 
   /**
-   * Null handling OFF: any unresolved row throws. One representative path is enough — the throw
-   * is independent of path shape and result type.
+   * 1.2 — NH on, 4-arg with `NULL` literal. Same result as 1.1, but exercises the `_defaultIsNull`
+   * code path in `getNullBitmap` (the SV transform writes the typed zero via `_defaultValue != null`,
+   * then `getNullBitmap` scans and marks unresolved rows null via the `!_defaultIsNull` carve-out).
+   */
+  @Test(dataProvider = "sqlNullDefaultProjectionCases")
+  public void testProjectionWithSqlNullDefaultEmitsNull(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC NULLS LAST", expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /**
+   * 1.3 — NH on, 4-arg with non-null default. The SV transform's priority is: real default >
+   * null-handling placeholder > throw. The user-supplied default surfaces for unresolved rows;
+   * no null bit is set in the bitmap.
+   * 
+   *   Example — flatJson.country with default 'foobar':
+   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar", row 3 -> "foobar"
+   *
+   *     Query: SET enableNullHandling = true;
+   *            SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c
+   *            FROM clicks ORDER BY c ASC
+   *
+   *     Result (×2 dup; ASCII order): "CA", "CA", "US", "US", "foobar"×4
+   * 

    */
+  @Test(dataProvider = "defaultPrecedenceProjectionCases")
+  public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
+      String defaultLiteral, Object[][] expectedRows, String label) {
+    String expr = String.format(
+        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /** 1.4 — NH off: any unresolved row throws. One representative path; throw is path-independent. */
   @Test
   public void testProjectionNullHandlingOffThrows() {
     try {
@@ -1034,11 +1077,7 @@ public void testProjectionNullHandlingOffThrows() {
     }
   }
 
-  /**
-   * Each row is `(column, jsonPath, resultsType, expected 8-row result, label)`. Result rows are
-   * ordered by value ASC NULLS LAST. With segment duplication, the per-row count in the fixture
-   * is ×2 in the result.
-   */
+  /** 1.1 cases — `(column, jsonPath, resultsType, expected 8-row result, label)`, ordered ASC NULLS LAST. */
   @DataProvider(name = "projectionCases")
   public static Object[][] projectionCases() {
     return new Object[][]{
@@ -1085,23 +1124,49 @@ public static Object[][] projectionCases() {
     };
   }
 
+  /** 1.2 cases — `(column, jsonPath, expected 8-row result ordered ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultProjectionCases")
+  public static Object[][] sqlNullDefaultProjectionCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", new Object[][]{
+            {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null}
+        }, "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{
+            {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null}
+        }, "nestedJson.location.country NULL default"}
+    };
+  }
+
+  /** 1.3 cases — `(column, jsonPath, default SQL literal, expected 8-row result ASC, label)`. */
+  @DataProvider(name = "defaultPrecedenceProjectionCases")
+  public static Object[][] defaultPrecedenceProjectionCases() {
+    return new Object[][]{
+        // flatJson.country: row0="US", row1="CA", row2=null, row3=missing -> 2 each, 4 'foobar'.
+        {"flatJson", "$.country", "'foobar'", new Object[][]{
+            {"CA"}, {"CA"}, {"US"}, {"US"},
+            {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}
+        }, "flatJson.country default 'foobar'"},
+        // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 'foobar'.
+        {"nestedJson", "$.location.country", "'foobar'", new Object[][]{
+            {"US"}, {"US"},
+            {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}
+        }, "nestedJson.location.country default 'foobar'"}
+    };
+  }
+
   // ============================================================================================
   // 2. DISTINCT
+  //
+  // Pinot rejects positional `ORDER BY 1` for DISTINCT ("ORDER-BY columns should be included in
+  // the DISTINCT columns"), so each DISTINCT test repeats the projected expression in ORDER BY.
   // ============================================================================================
 
   /**
-   * DISTINCT with null handling ON. The distinct set must include exactly one null entry
-   * alongside the resolved values (deduped across segments).
+   * 2.1 — NH on, 3-arg. The distinct set must include exactly one null entry alongside the
+   * resolved values (deduped across segments).
    * 
-   *   Example — `flatJson.country` (STRING):
-   *     Query:  SET enableNullHandling = true;
-   *             SELECT DISTINCT jsonExtractScalar(flatJson, '$.country', 'STRING') FROM clicks
-   *             ORDER BY jsonExtractScalar(flatJson, '$.country', 'STRING') ASC NULLS LAST
-   *
-   *     Result: "CA", "US", NULL
+   *   Example — `flatJson.country`: Result: "CA", "US", NULL
    * 

-   * Pinot rejects positional `ORDER BY 1` for DISTINCT ("ORDER-BY columns should be included in
-   * the DISTINCT columns"), so the expression is repeated in the ORDER BY clause.
    */
   @Test(dataProvider = "distinctCases")
   public void testDistinctNullHandlingOn(String column, String jsonPath, String resultsType,
@@ -1111,6 +1176,28 @@ public void testDistinctNullHandlingOn(String column, String jsonPath, String re
     givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows);
   }
 
+  /** 2.2 — NH on, 4-arg with `NULL` literal. Same distinct set as 2.1 (includes a null entry). */
+  @Test(dataProvider = "sqlNullDefaultDistinctCases")
+  public void testDistinctWithSqlNullDefaultIncludesNull(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /** 2.3 — NH on, 4-arg with non-null default. Default value appears as a regular distinct entry (no null). */
+  @Test(dataProvider = "defaultPrecedenceDistinctCases")
+  public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
+      String defaultLiteral, Object[][] expectedRows, String label) {
+    String expr = String.format(
+        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /** 2.4 — NH off: DISTINCT throws on unresolved rows. */
   @Test
   public void testDistinctNullHandlingOffThrows() {
     try {
@@ -1123,7 +1210,7 @@ public void testDistinctNullHandlingOffThrows() {
     }
   }
 
-  /** Each row is `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */
+  /** 2.1 cases — `(column, jsonPath, resultsType, expected distinct rows ASC NULLS LAST, label)`. */
   @DataProvider(name = "distinctCases")
   public static Object[][] distinctCases() {
     return new Object[][]{
@@ -1140,33 +1227,75 @@ public static Object[][] distinctCases() {
     };
   }
 
+  /** 2.2 cases — `(column, jsonPath, expected distinct rows ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultDistinctCases")
+  public static Object[][] sqlNullDefaultDistinctCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{{"US"}, {null}},
+            "nestedJson.location.country NULL default"}
+    };
+  }
+
+  /** 2.3 cases — `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */
+  @DataProvider(name = "defaultPrecedenceDistinctCases")
+  public static Object[][] defaultPrecedenceDistinctCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", "'foobar'",
+            new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"},
+        {"nestedJson", "$.location.country", "'foobar'",
+            new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"}
+    };
+  }
+
   // ============================================================================================
   // 3. GROUP BY
   // ============================================================================================
 
   /**
-   * GROUP BY with null handling ON. Unresolved rows must collapse into a single null group with
-   * the correct count, alongside the resolved value groups.
+   * 3.1 — NH on, 3-arg. Unresolved rows collapse into a single null group with the correct count,
+   * alongside the resolved value groups.
    * 
-   *   Example — `flatJson.country` (STRING):
-   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> NULL (explicit), row 3 -> NULL (missing)
-   *
-   *     Query:  SET enableNullHandling = true;
-   *             SELECT jsonExtractScalar(flatJson, '$.country', 'STRING') AS v, COUNT(*)
-   *             FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST
-   *
-   *     Result (counts ×2 for segment duplication): ("CA", 2), ("US", 2), (NULL, 4)
+   *   Example — `flatJson.country`: Result (counts ×2 for segment dup): ("CA", 2), ("US", 2), (NULL, 4)
    * 

    */
   @Test(dataProvider = "groupByCases")
   public void testGroupByNullHandlingOn(String column, String jsonPath, String resultsType,
       Object[][] expectedRows, String label) {
     String expr = String.format("jsonExtractScalar(%s, '%s', '%s')", column, jsonPath, resultsType);
-    String query = String.format(
-        "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr);
-    givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows);
+    givenCountryClickTable(true)
+        .whenQuery(String.format(
+            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /** 3.2 — NH on, 4-arg with `NULL` literal. Same groups as 3.1 (null group with correct count). */
+  @Test(dataProvider = "sqlNullDefaultGroupByCases")
+  public void testGroupByWithSqlNullDefaultProducesNullGroup(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format(
+            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /**
+   * 3.3 — NH on, 4-arg with non-null default. Unresolved rows count toward the default's group,
+   * not a null group — so the result has no NULL group at all.
+   */
+  @Test(dataProvider = "defaultPrecedenceGroupByCases")
+  public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
+      String defaultLiteral, Object[][] expectedRows, String label) {
+    String expr = String.format(
+        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
+    givenCountryClickTable(true)
+        .whenQuery(String.format(
+            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr))
+        .thenResultIs(expectedRows);
   }
 
+  /** 3.4 — NH off: GROUP BY throws on unresolved rows. */
   @Test
   public void testGroupByNullHandlingOffThrows() {
     try {
@@ -1179,10 +1308,7 @@ public void testGroupByNullHandlingOffThrows() {
     }
   }
 
-  /**
-   * Each row is `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST,
-   * label)`. Counts are ×2 the per-row count due to segment duplication.
-   */
+  /** 3.1 cases — `(column, jsonPath, resultsType, expected (value, count) rows ASC NULLS LAST, label)`. */
   @DataProvider(name = "groupByCases")
   public static Object[][] groupByCases() {
     return new Object[][]{
@@ -1209,103 +1335,18 @@ public static Object[][] groupByCases() {
     };
   }
 
-  // ============================================================================================
-  // 4. Default-value precedence under null handling ON
-  //
-  // When a non-null default literal is supplied AND null handling is on, the SV transform's
-  // priority order is: real default > null-handling placeholder > throw. The user-supplied
-  // default surfaces for unresolved rows; the null-handling placeholder is NOT emitted, and
-  // no null bit is set in the bitmap. These tests pin that ordering across projection,
-  // DISTINCT, and GROUP BY so a future refactor can't silently swap the priority.
-  // ============================================================================================
-
-  /**
-   * Projection with NH on AND a non-null default literal. Unresolved rows must surface as the
-   * default, not SQL NULL — across both flat scalar and nested-object shapes.
-   * 
-   *   Example — flatJson.country with default 'foobar':
-   *     per-row: row 0 -> "US", row 1 -> "CA", row 2 -> "foobar" (was null),
-   *              row 3 -> "foobar" (was missing)
-   *
-   *     Query: SET enableNullHandling = true;
-   *            SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', 'foobar') AS c
-   *            FROM clicks ORDER BY c ASC
-   *
-   *     Result rows (×2 segment dup; ASCII order, uppercase before lowercase):
-   *             "CA", "CA", "US", "US", "foobar", "foobar", "foobar", "foobar"
-   * 

-   */
-  @Test(dataProvider = "defaultPrecedenceProjectionCases")
-  public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
-      String defaultLiteral, Object[][] expectedRows, String label) {
-    String expr = String.format(
-        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
-    givenCountryClickTable(true)
-        .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC", expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /**
-   * Each row: `(column, jsonPath, default SQL literal, expected 8-row result, label)`. Result
-   * rows are ordered ASC. Each per-row value in the fixture appears ×2 due to segment duplication.
-   */
-  @DataProvider(name = "defaultPrecedenceProjectionCases")
-  public static Object[][] defaultPrecedenceProjectionCases() {
-    return new Object[][]{
-        // flatJson.country: row0="US", row1="CA", row2=null, row3=missing -> 2 each, 4 'foobar'.
-        {"flatJson", "$.country", "'foobar'", new Object[][]{
-            {"CA"}, {"CA"}, {"US"}, {"US"},
-            {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}
-        }, "flatJson.country default 'foobar'"},
-        // nestedJson.location.country: row0="US", rows 1/2/3 unresolved -> 2 "US" + 6 'foobar'.
-        {"nestedJson", "$.location.country", "'foobar'", new Object[][]{
-            {"US"}, {"US"},
-            {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}, {"foobar"}
-        }, "nestedJson.location.country default 'foobar'"}
-    };
-  }
-
-  /**
-   * DISTINCT with NH on AND a non-null default literal. The distinct set includes the default
-   * value as a regular entry — no separate null entry.
-   */
-  @Test(dataProvider = "defaultPrecedenceDistinctCases")
-  public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
-      String defaultLiteral, Object[][] expectedRows, String label) {
-    String expr = String.format(
-        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
-    givenCountryClickTable(true)
-        .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC", expr, expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /** Each row: `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */
-  @DataProvider(name = "defaultPrecedenceDistinctCases")
-  public static Object[][] defaultPrecedenceDistinctCases() {
+  /** 3.2 cases — `(column, jsonPath, expected (value, count) rows ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultGroupByCases")
+  public static Object[][] sqlNullDefaultGroupByCases() {
     return new Object[][]{
-        {"flatJson", "$.country", "'foobar'",
-            new Object[][]{{"CA"}, {"US"}, {"foobar"}}, "flatJson.country default 'foobar'"},
-        {"nestedJson", "$.location.country", "'foobar'",
-            new Object[][]{{"US"}, {"foobar"}}, "nestedJson.location.country default 'foobar'"}
+        {"flatJson", "$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}},
+            "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{{"US", 2L}, {null, 6L}},
+            "nestedJson.location.country NULL default"}
     };
   }
 
-  /**
-   * GROUP BY with NH on AND a non-null default literal. Unresolved rows count toward the
-   * default's group — no NULL group surfaces.
-   */
-  @Test(dataProvider = "defaultPrecedenceGroupByCases")
-  public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
-      String defaultLiteral, Object[][] expectedRows, String label) {
-    String expr = String.format(
-        "jsonExtractScalar(%s, '%s', 'STRING', %s)", column, jsonPath, defaultLiteral);
-    givenCountryClickTable(true)
-        .whenQuery(String.format(
-            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC", expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /** Each row: `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */
+  /** 3.3 cases — `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */
   @DataProvider(name = "defaultPrecedenceGroupByCases")
   public static Object[][] defaultPrecedenceGroupByCases() {
     return new Object[][]{
@@ -1319,92 +1360,4 @@ public static Object[][] defaultPrecedenceGroupByCases() {
         }, "nestedJson.location.country default 'foobar'"}
     };
   }
-
-  // ============================================================================================
-  // 5. 4-arg with SQL NULL literal as default, under null handling ON
-  //
-  // Result is identical to the 3-arg + NH-on case (unresolved rows surface as SQL NULL), but
-  // the code path is different: `init` sets `_defaultValue` to the typed zero (0, "", etc.) and
-  // marks `_defaultIsNull=true`. The SV transform writes the typed zero via the
-  // `_defaultValue != null` branch; `getNullBitmap` scans and marks unresolved rows null thanks
-  // to the `!_defaultIsNull` carve-out in its short-circuit condition. These tests pin that
-  // code path so a future refactor that removes `_defaultIsNull` won't silently break this case.
-  // ============================================================================================
-
-  /**
-   * Projection with NH on AND `NULL` literal as the default. Unresolved rows surface as SQL NULL,
-   * same as the 3-arg form.
-   * 
-   *   Example — flatJson.country:
-   *     Query: SET enableNullHandling = true;
-   *            SELECT jsonExtractScalar(flatJson, '$.country', 'STRING', NULL) AS c
-   *            FROM clicks ORDER BY c ASC NULLS LAST
-   *
-   *     Result rows (×2 segment dup):
-   *             "CA", "CA", "US", "US", NULL, NULL, NULL, NULL
-   * 

-   */
-  @Test(dataProvider = "sqlNullDefaultProjectionCases")
-  public void testProjectionWithSqlNullDefaultEmitsNull(String column, String jsonPath,
-      Object[][] expectedRows, String label) {
-    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
-    givenCountryClickTable(true)
-        .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC NULLS LAST", expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /** Each row: `(column, jsonPath, expected 8-row result ordered ASC NULLS LAST, label)`. */
-  @DataProvider(name = "sqlNullDefaultProjectionCases")
-  public static Object[][] sqlNullDefaultProjectionCases() {
-    return new Object[][]{
-        {"flatJson", "$.country", new Object[][]{
-            {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null}
-        }, "flatJson.country NULL default"},
-        {"nestedJson", "$.location.country", new Object[][]{
-            {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null}
-        }, "nestedJson.location.country NULL default"}
-    };
-  }
-
-  /** DISTINCT with NH on AND `NULL` literal as default — the distinct set includes a null entry. */
-  @Test(dataProvider = "sqlNullDefaultDistinctCases")
-  public void testDistinctWithSqlNullDefaultIncludesNull(String column, String jsonPath,
-      Object[][] expectedRows, String label) {
-    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
-    givenCountryClickTable(true)
-        .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /** Each row: `(column, jsonPath, expected distinct rows ASC NULLS LAST, label)`. */
-  @DataProvider(name = "sqlNullDefaultDistinctCases")
-  public static Object[][] sqlNullDefaultDistinctCases() {
-    return new Object[][]{
-        {"flatJson", "$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country NULL default"},
-        {"nestedJson", "$.location.country", new Object[][]{{"US"}, {null}},
-            "nestedJson.location.country NULL default"}
-    };
-  }
-
-  /** GROUP BY with NH on AND `NULL` literal as default — unresolved rows form a null group. */
-  @Test(dataProvider = "sqlNullDefaultGroupByCases")
-  public void testGroupByWithSqlNullDefaultProducesNullGroup(String column, String jsonPath,
-      Object[][] expectedRows, String label) {
-    String expr = String.format("jsonExtractScalar(%s, '%s', 'STRING', NULL)", column, jsonPath);
-    givenCountryClickTable(true)
-        .whenQuery(String.format(
-            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr))
-        .thenResultIs(expectedRows);
-  }
-
-  /** Each row: `(column, jsonPath, expected (value, count) rows ASC NULLS LAST, label)`. */
-  @DataProvider(name = "sqlNullDefaultGroupByCases")
-  public static Object[][] sqlNullDefaultGroupByCases() {
-    return new Object[][]{
-        {"flatJson", "$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}},
-            "flatJson.country NULL default"},
-        {"nestedJson", "$.location.country", new Object[][]{{"US", 2L}, {null, 6L}},
-            "nestedJson.location.country NULL default"}
-    };
-  }
 }

From 067a22c8c6577ce5691d52cf6df50ac82d229e6a Mon Sep 17 00:00:00 2001
From: Siddharth Teotia 
Date: Wed, 27 May 2026 15:11:12 -0700
Subject: [PATCH 10/14] Address code-review nits in
 JsonExtractIndexTransformFunction

- Cache the per-ValueBlock getValuesSV result so the SV transform and
  getNullBitmap don't each hit the index reader under null handling
  (perf MAJOR).
- Add null-handling note to the class Javadoc (doc MAJOR).
- Use valueBlock.getNumDocs() for the loop bound in getNullBitmap to
  match sibling SV transforms (MINOR).
- Swap @Override @Nullable to @Nullable @Override to match the parent's
  convention (MINOR).
---
 .../JsonExtractIndexTransformFunction.java    | 52 +++++++++++++------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
index d5eb59ed63..289bfe9ba5 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
@@ -43,6 +43,12 @@
  * implementation changed to read values from the JSON index. For large JSON blobs this can be faster than parsing
  * GBs of JSON at query time. For small JSON blobs/highly filtered input this is generally slower than the *scalar
  * implementation. The inflection point is highly dependent on the number of docs remaining post filter.
+ *
+ * 
Null handling: when query-level null handling is enabled (e.g. {@code SET enableNullHandling = true})
+ * and no default literal is supplied, an unresolved JSON path on a row surfaces as SQL {@code NULL} via the
+ * {@link #getNullBitmap(ValueBlock)} bitmap rather than throwing. With null handling disabled, the legacy throw
+ * is preserved. A user-supplied non-null default literal takes precedence and is written for unresolved rows
+ * regardless of null-handling state.
  */
 public class JsonExtractIndexTransformFunction extends BaseTransformFunction {
   public static final String FUNCTION_NAME = "jsonExtractIndex";
@@ -56,6 +62,13 @@ public class JsonExtractIndexTransformFunction extends BaseTransformFunction {
   private boolean _isSingleValue;
   private String _filterJsonPath;
 
+  // Cache for the per-ValueBlock SV scan result. Both the SV transforms and getNullBitmap need
+  // the same `String[]` for the same ValueBlock; without caching, we'd hit the JSON index twice
+  // per block on the null-handling path. Identity comparison is safe — the broker constructs a
+  // fresh ValueBlock per batch and never mutates a previously returned one.
+  private ValueBlock _cachedValueBlock;
+  private String[] _cachedValuesSV;
+
   @Override
   public String getName() {
     return FUNCTION_NAME;
@@ -162,8 +175,7 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initIntValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -188,8 +200,7 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initLongValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -214,8 +225,7 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initFloatValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -240,8 +250,7 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initDoubleValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -266,8 +275,7 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initBigDecimalValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -292,8 +300,7 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
     int numDocs = valueBlock.getNumDocs();
     int[] inputDocIds = valueBlock.getDocIds();
     initStringValuesSV(numDocs);
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
     for (int i = 0; i < numDocs; i++) {
       String value = valuesFromIndex[i];
       if (value == null) {
@@ -313,8 +320,8 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
     return _stringValuesSV;
   }
 
-  @Override
   @Nullable
+  @Override
   public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
     // Short-circuit to argument-bitmap propagation when this function isn't introducing nulls of
     // its own: non-SV output, null handling disabled, or any default literal supplied (the SV
@@ -324,10 +331,10 @@ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
     if (!_isSingleValue || !_nullHandlingEnabled || _defaultValue != null) {
       return super.getNullBitmap(valueBlock);
     }
-    String[] valuesFromIndex = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
-        getValueToMatchingDocsMap(), false);
+    String[] valuesFromIndex = getCachedValuesFromIndex(valueBlock);
+    int numDocs = valueBlock.getNumDocs();
     RoaringBitmap nullBitmap = new RoaringBitmap();
-    for (int i = 0; i < valuesFromIndex.length; i++) {
+    for (int i = 0; i < numDocs; i++) {
       if (valuesFromIndex[i] == null) {
         nullBitmap.add(i);
       }
@@ -482,4 +489,17 @@ private Map getValueToMatchingDocsMap() {
     }
     return _valueToMatchingDocsMap;
   }
+
+  /**
+   * Returns the JSON-index SV scan result for the given block, caching it so that the SV
+   * transform and {@link #getNullBitmap} don't each pay the cost on the null-handling path.
+   */
+  private String[] getCachedValuesFromIndex(ValueBlock valueBlock) {
+    if (valueBlock != _cachedValueBlock) {
+      _cachedValuesSV = _jsonIndexReader.getValuesSV(valueBlock.getDocIds(), valueBlock.getNumDocs(),
+          getValueToMatchingDocsMap(), false);
+      _cachedValueBlock = valueBlock;
+    }
+    return _cachedValuesSV;
+  }
 }

From 29c9c66772190bb7be95ec32d20b6174c2cbb568 Mon Sep 17 00:00:00 2001
From: Siddharth Teotia 
Date: Wed, 27 May 2026 15:37:50 -0700
Subject: [PATCH 11/14] Honor SQL NULL literal default in jsonExtractIndex,
 mirroring scalar

init now detects a SQL NULL literal as the 4-arg default via literal.isNull()
and leaves _defaultValue null, so 3-arg semantics apply (NH on -> SQL NULL,
NH off -> throw). Without this, STRING/JSON silently emitted "" and numerics
failed at init -- both inconsistent with the scalar function's fix.

Adds matching Section 1.2 / 2.2 / 3.2 tests in the index test file (6 new
parameterized cases) covering flat + nested paths under projection, DISTINCT,
and GROUP BY. Also fixes a comment typo (broker -> server) flagged by review.
---
 .../JsonExtractIndexTransformFunction.java    |  43 ++++----
 ...JsonExtractIndexTransformFunctionTest.java | 103 +++++++++++++++---
 2 files changed, 110 insertions(+), 36 deletions(-)

diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
index 289bfe9ba5..a0c9ad9781 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
@@ -64,7 +64,7 @@ public class JsonExtractIndexTransformFunction extends BaseTransformFunction {
 
   // Cache for the per-ValueBlock SV scan result. Both the SV transforms and getNullBitmap need
   // the same `String[]` for the same ValueBlock; without caching, we'd hit the JSON index twice
-  // per block on the null-handling path. Identity comparison is safe — the broker constructs a
+  // per block on the null-handling path. Identity comparison is safe — the server constructs a
   // fresh ValueBlock per batch and never mutates a previously returned one.
   private ValueBlock _cachedValueBlock;
   private String[] _cachedValuesSV;
@@ -134,22 +134,27 @@ public void init(List arguments, Map c
       if (!(fourthArgument instanceof LiteralTransformFunction)) {
         throw new IllegalArgumentException("Default value must be a literal");
       }
-
-      if (_isSingleValue) {
-        _defaultValue = dataType.convert(((LiteralTransformFunction) fourthArgument).getStringLiteral());
-      } else {
-        try {
-          JsonNode mvArray = JsonUtils.stringToJsonNode(((LiteralTransformFunction) fourthArgument).getStringLiteral());
-          if (!mvArray.isArray()) {
+      LiteralTransformFunction literalFn = (LiteralTransformFunction) fourthArgument;
+      // SQL NULL literal as the default ⇒ behave like the 3-arg form: leave `_defaultValue` null
+      // so unresolved rows surface as SQL NULL (NH on) or throw (NH off). This mirrors the scalar
+      // function's `_defaultIsNull` handling without needing a separate flag.
+      if (!literalFn.isNull()) {
+        if (_isSingleValue) {
+          _defaultValue = dataType.convert(literalFn.getStringLiteral());
+        } else {
+          try {
+            JsonNode mvArray = JsonUtils.stringToJsonNode(literalFn.getStringLiteral());
+            if (!mvArray.isArray()) {
+              throw new IllegalArgumentException("Default value must be a valid JSON array");
+            }
+            Object[] defaultValues = new Object[mvArray.size()];
+            for (int i = 0; i < mvArray.size(); i++) {
+              defaultValues[i] = dataType.convert(mvArray.get(i).asText());
+            }
+            _defaultValue = defaultValues;
+          } catch (IOException e) {
             throw new IllegalArgumentException("Default value must be a valid JSON array");
           }
-          Object[] defaultValues = new Object[mvArray.size()];
-          for (int i = 0; i < mvArray.size(); i++) {
-            defaultValues[i] = dataType.convert(mvArray.get(i).asText());
-          }
-          _defaultValue = defaultValues;
-        } catch (IOException e) {
-          throw new IllegalArgumentException("Default value must be a valid JSON array");
         }
       }
     }
@@ -324,10 +329,10 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
   @Override
   public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
     // Short-circuit to argument-bitmap propagation when this function isn't introducing nulls of
-    // its own: non-SV output, null handling disabled, or any default literal supplied (the SV
-    // transform writes it for unresolved rows). Unlike the scalar function, the parser converts a
-    // SQL-NULL literal to the typed zero ("" for STRING, throws at init for numerics), so there's
-    // no SQL-NULL-default path to fall through.
+    // its own: non-SV output, null handling disabled, or a non-null default literal supplied (the
+    // SV transform writes it for unresolved rows). The SQL NULL literal case is handled in `init`
+    // by leaving `_defaultValue` null so it falls through to the scan below — same as the 3-arg
+    // form.
     if (!_isSingleValue || !_nullHandlingEnabled || _defaultValue != null) {
       return super.getNullBitmap(valueBlock);
     }
diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java
index d917a17259..5167c66b84 100644
--- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunctionTest.java
@@ -457,17 +457,18 @@ private  T getValueForKey(String blob, JsonPath path, TypeRef typeRef) {
   //                  resolved / null / missing data at depth.
   //
   // Tests are grouped by query shape in this order: projection → DISTINCT → GROUP BY. Each section
-  // covers three sub-cases, in order:
+  // covers the same four sub-cases, in order:
   //
   //   .1  NH on, 3-arg                          — unresolved rows surface as SQL NULL (E4 fix)
-  //   .2  NH on, 4-arg with a non-null default   — default wins over NH placeholder
-  //   .3  NH off                                  — legacy throw preserved
+  //   .2  NH on, 4-arg with SQL NULL literal     — same result as .1; `init` detects the null
+  //                                                literal and leaves `_defaultValue` null so the
+  //                                                3-arg semantics apply
+  //   .3  NH on, 4-arg with a non-null default   — default wins over NH placeholder
+  //   .4  NH off                                  — legacy throw preserved
   //
-  // Unlike the scalar function, the index function has no `_defaultIsNull` path: a `NULL` literal
-  // as the 4-arg default fails at init (numeric) or silently becomes empty string (STRING) — both
-  // pre-existing quirks outside the scope of this PR. Result rows in every assertion are ordered
-  // by the projected expression ASC NULLS LAST so the comparison is deterministic across the
-  // framework's segment-duplication behavior (one segment becomes two, so per-row counts ×2).
+  // Result rows in every assertion are ordered by the projected expression ASC NULLS LAST so the
+  // comparison is deterministic across the framework's segment-duplication behavior (one segment
+  // becomes two, so per-row counts ×2).
   // ============================================================================================
 
   protected File _baseDir;
@@ -590,7 +591,20 @@ public void testProjectionNullHandlingOn(String column, String jsonPath, String
   }
 
   /**
-   * 1.2 — NH on, 4-arg with non-null default. The SV transform's priority is: real default >
+   * 1.2 — NH on, 4-arg with `NULL` literal. Same result as 1.1: `init` detects the null literal
+   * and leaves `_defaultValue` null, so the 3-arg code path applies.
+   */
+  @Test(dataProvider = "sqlNullDefaultProjectionCases")
+  public void testProjectionWithSqlNullDefaultEmitsNull(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractIndex(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT %s AS c FROM clicks ORDER BY c ASC NULLS LAST", expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /**
+   * 1.3 — NH on, 4-arg with non-null default. The SV transform's priority is: real default >
    * null-handling placeholder > throw. The user-supplied default surfaces for unresolved rows;
    * no null bit is set in the bitmap.
    */
@@ -605,7 +619,7 @@ public void testProjectionDefaultBeatsNullHandlingPlaceholder(String column, Str
   }
 
   /**
-   * 1.3 — NH off: any unresolved row throws. One representative path; throw is path-independent.
+   * 1.4 — NH off: any unresolved row throws. One representative path; throw is path-independent.
    * The index function's error message differs from the scalar's ("Illegal Json Path" vs "Cannot
    * resolve JSON path").
    */
@@ -664,7 +678,20 @@ public static Object[][] projectionCases() {
     };
   }
 
-  /** 1.2 cases — `(column, jsonPath, default SQL literal, expected 8-row result ASC, label)`. */
+  /** 1.2 cases — `(column, jsonPath, expected 8-row result ordered ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultProjectionCases")
+  public static Object[][] sqlNullDefaultProjectionCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", new Object[][]{
+            {"CA"}, {"CA"}, {"US"}, {"US"}, {null}, {null}, {null}, {null}
+        }, "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{
+            {"US"}, {"US"}, {null}, {null}, {null}, {null}, {null}, {null}
+        }, "nestedJson.location.country NULL default"}
+    };
+  }
+
+  /** 1.3 cases — `(column, jsonPath, default SQL literal, expected 8-row result ASC, label)`. */
   @DataProvider(name = "defaultPrecedenceProjectionCases")
   public static Object[][] defaultPrecedenceProjectionCases() {
     return new Object[][]{
@@ -698,7 +725,17 @@ public void testDistinctNullHandlingOn(String column, String jsonPath, String re
     givenCountryClickTable(true).whenQuery(query).thenResultIs(expectedRows);
   }
 
-  /** 2.2 — NH on, 4-arg with non-null default. Default appears as a regular distinct entry (no null). */
+  /** 2.2 — NH on, 4-arg with `NULL` literal. Same distinct set as 2.1 (includes a null entry). */
+  @Test(dataProvider = "sqlNullDefaultDistinctCases")
+  public void testDistinctWithSqlNullDefaultIncludesNull(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractIndex(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format("SELECT DISTINCT %s FROM clicks ORDER BY %s ASC NULLS LAST", expr, expr))
+        .thenResultIs(expectedRows);
+  }
+
+  /** 2.3 — NH on, 4-arg with non-null default. Default appears as a regular distinct entry (no null). */
   @Test(dataProvider = "defaultPrecedenceDistinctCases")
   public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, String jsonPath,
       String defaultLiteral, Object[][] expectedRows, String label) {
@@ -709,7 +746,7 @@ public void testDistinctDefaultBeatsNullHandlingPlaceholder(String column, Strin
         .thenResultIs(expectedRows);
   }
 
-  /** 2.3 — NH off: DISTINCT throws on unresolved rows. */
+  /** 2.4 — NH off: DISTINCT throws on unresolved rows. */
   @Test
   public void testDistinctNullHandlingOffThrows() {
     try {
@@ -737,7 +774,17 @@ public static Object[][] distinctCases() {
     };
   }
 
-  /** 2.2 cases — `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */
+  /** 2.2 cases — `(column, jsonPath, expected distinct rows ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultDistinctCases")
+  public static Object[][] sqlNullDefaultDistinctCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", new Object[][]{{"CA"}, {"US"}, {null}}, "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{{"US"}, {null}},
+            "nestedJson.location.country NULL default"}
+    };
+  }
+
+  /** 2.3 cases — `(column, jsonPath, default literal, expected distinct rows ASC, label)`. */
   @DataProvider(name = "defaultPrecedenceDistinctCases")
   public static Object[][] defaultPrecedenceDistinctCases() {
     return new Object[][]{
@@ -766,8 +813,19 @@ public void testGroupByNullHandlingOn(String column, String jsonPath, String res
         .thenResultIs(expectedRows);
   }
 
+  /** 3.2 — NH on, 4-arg with `NULL` literal. Same groups as 3.1 (null group with correct count). */
+  @Test(dataProvider = "sqlNullDefaultGroupByCases")
+  public void testGroupByWithSqlNullDefaultProducesNullGroup(String column, String jsonPath,
+      Object[][] expectedRows, String label) {
+    String expr = String.format("jsonExtractIndex(%s, '%s', 'STRING', NULL)", column, jsonPath);
+    givenCountryClickTable(true)
+        .whenQuery(String.format(
+            "SELECT %s AS v, COUNT(*) FROM clicks GROUP BY v ORDER BY v ASC NULLS LAST", expr))
+        .thenResultIs(expectedRows);
+  }
+
   /**
-   * 3.2 — NH on, 4-arg with non-null default. Unresolved rows count toward the default's group;
+   * 3.3 — NH on, 4-arg with non-null default. Unresolved rows count toward the default's group;
    * no NULL group surfaces.
    */
   @Test(dataProvider = "defaultPrecedenceGroupByCases")
@@ -781,7 +839,7 @@ public void testGroupByDefaultBeatsNullHandlingPlaceholder(String column, String
         .thenResultIs(expectedRows);
   }
 
-  /** 3.3 — NH off: GROUP BY throws on unresolved rows. */
+  /** 3.4 — NH off: GROUP BY throws on unresolved rows. */
   @Test
   public void testGroupByNullHandlingOffThrows() {
     try {
@@ -816,7 +874,18 @@ public static Object[][] groupByCases() {
     };
   }
 
-  /** 3.2 cases — `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */
+  /** 3.2 cases — `(column, jsonPath, expected (value, count) rows ASC NULLS LAST, label)`. */
+  @DataProvider(name = "sqlNullDefaultGroupByCases")
+  public static Object[][] sqlNullDefaultGroupByCases() {
+    return new Object[][]{
+        {"flatJson", "$.country", new Object[][]{{"CA", 2L}, {"US", 2L}, {null, 4L}},
+            "flatJson.country NULL default"},
+        {"nestedJson", "$.location.country", new Object[][]{{"US", 2L}, {null, 6L}},
+            "nestedJson.location.country NULL default"}
+    };
+  }
+
+  /** 3.3 cases — `(column, jsonPath, default literal, expected (value, count) rows ASC, label)`. */
   @DataProvider(name = "defaultPrecedenceGroupByCases")
   public static Object[][] defaultPrecedenceGroupByCases() {
     return new Object[][]{

From c1c9db6fcf2759fcac14799e347fd02095835dac Mon Sep 17 00:00:00 2001
From: siddharthteotia 
Date: Fri, 29 May 2026 15:28:35 -0700
Subject: [PATCH 12/14] Adapt JsonExtractIndex init to li-pinot's QueryContext
 signature

Mirrors the prereqs-PR adaptation for JsonExtractScalar. The 11
upstream feature commits target BaseTransformFunction.init's
boolean nullHandlingEnabled argument; li-pinot's BaseTransformFunction
uses QueryContext instead, so we propagate the type change here.
_nullHandlingEnabled is still inherited from the base class via
super.init.
---
 .../function/JsonExtractIndexTransformFunction.java          | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
index a0c9ad9781..2386750ae8 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractIndexTransformFunction.java
@@ -29,6 +29,7 @@
 import org.apache.pinot.core.operator.ColumnContext;
 import org.apache.pinot.core.operator.blocks.ValueBlock;
 import org.apache.pinot.core.operator.transform.TransformResultMetadata;
+import org.apache.pinot.core.query.request.context.QueryContext;
 import org.apache.pinot.segment.spi.index.IndexService;
 import org.apache.pinot.segment.spi.index.IndexType;
 import org.apache.pinot.segment.spi.index.reader.JsonIndexReader;
@@ -76,8 +77,8 @@ public String getName() {
 
   @Override
   public void init(List arguments, Map columnContextMap,
-      boolean nullHandlingEnabled) {
-    super.init(arguments, columnContextMap, nullHandlingEnabled);
+      QueryContext queryContext) {
+    super.init(arguments, columnContextMap, queryContext);
     // Check that there are exactly 3 or 4 or 5 arguments
     if (arguments.size() < 3 || arguments.size() > 5) {
       throw new IllegalArgumentException(

From c0ff474cfbd278ceda50e273fe86c27b19339965 Mon Sep 17 00:00:00 2001
From: Siddharth Teotia 
Date: Mon, 8 Jun 2026 11:43:49 -0700
Subject: [PATCH 13/14] =?UTF-8?q?WIP:=20Narrow=20JSON-extract=20catch=20?=
 =?UTF-8?q?=E2=80=94=20distinguish=20parse=20failure=20from=20path=20miss?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PR #205's null-handling broadcasted past its intended scope: the existing
broad `catch (Exception ignored)` swallowed jayway's `InvalidJsonException`
(input isn't JSON at all) alongside the legitimate `PathNotFoundException`
(valid JSON, path absent), then coerced both to SQL NULL under NH-on. This
silently masked schema modeling errors — e.g. `jsonExtractScalar` invoked on
a non-JSON STRING column returned 15 rows of NULL instead of surfacing the
caller bug.

Split the catch arms in all 12 transform sites:
  - PathNotFoundException → silent; the existing null-handling / default /
    throw cascade decides per row (PR #205's feature, preserved).
  - InvalidJsonException → throw IllegalArgumentException with a clear
    "received input that is not valid JSON" message, regardless of NH state.

Updates the upstream `QueryRunnerTest` case #12 expectation to match the new
contract, and adds two new tests in `JsonExtractScalarTransformFunctionTest`
locking in "non-JSON input throws under both NH on and off".

JsonExtractIndex is deliberately untouched — it already rejects non-JSON
columns at init() via the "must have JSON index" precondition.

Known open question: local Temurin-21 run does not reproduce the CI failure
(test passes locally pre- and post-patch). CI-vs-local divergence likely
TestNG ordering / fixture state; pushing for empirical CI signal. Fallback
if InvalidJsonException doesn't fire under CI conditions is to remove
`Option.SUPPRESS_EXCEPTIONS` from the parser contexts (documented in
CLAUDE.local.md resume notes).

Co-Authored-By: Claude Opus 4.7 
---
 .../JsonExtractScalarTransformFunction.java   | 122 ++++++++++++++++--
 ...sonExtractScalarTransformFunctionTest.java |  48 +++++++
 .../runtime/queries/QueryRunnerTest.java      |  11 +-
 3 files changed, 165 insertions(+), 16 deletions(-)

diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
index 446a63a947..73f7448028 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
@@ -22,9 +22,11 @@
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.jayway.jsonpath.Configuration;
+import com.jayway.jsonpath.InvalidJsonException;
 import com.jayway.jsonpath.JsonPath;
 import com.jayway.jsonpath.Option;
 import com.jayway.jsonpath.ParseContext;
+import com.jayway.jsonpath.PathNotFoundException;
 import com.jayway.jsonpath.spi.json.JacksonJsonProvider;
 import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
 import java.math.BigDecimal;
@@ -216,7 +218,15 @@ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         nullBitmap.add(i);
@@ -245,7 +255,15 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -280,7 +298,15 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -312,7 +338,15 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -344,7 +378,15 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -376,7 +418,15 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -408,7 +458,15 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -441,7 +499,15 @@ public int[][] transformToIntValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         _intValuesMV[i] = new int[0];
@@ -481,7 +547,15 @@ public long[][] transformToLongValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         _longValuesMV[i] = new long[0];
@@ -520,7 +594,15 @@ public float[][] transformToFloatValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         _floatValuesMV[i] = new float[0];
@@ -559,7 +641,15 @@ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         _doubleValuesMV[i] = new double[0];
@@ -598,7 +688,15 @@ public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (Exception ignored) {
+      } catch (PathNotFoundException ignored) {
+        // result stays null; the cascade below applies null-handling or the default value.
+      } catch (InvalidJsonException e) {
+        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
+        // invoked on a non-JSON column), not a missing value — surface it regardless of
+        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        throw new IllegalArgumentException(
+            "jsonExtractScalar received input that is not valid JSON. "
+                + "This function requires a JSON-typed column or a JSON-formatted string.", e);
       }
       if (result == null) {
         _stringValuesMV[i] = new String[0];
diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
index e49f7f6eaa..9f626bfbd5 100644
--- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
@@ -1360,4 +1360,52 @@ public static Object[][] defaultPrecedenceGroupByCases() {
         }, "nestedJson.location.country default 'foobar'"}
     };
   }
+
+  // ============================================================================================
+  // 4. Non-JSON input rejection
+  // ============================================================================================
+  // jsonExtractScalar invoked on a column whose values are not parseable as JSON is a caller /
+  // schema modeling error, not a missing value. The function must throw regardless of the null
+  // handling setting — coercing parse failures to SQL NULL would silently mask the bug. Null
+  // handling applies only to valid-JSON-with-missing-path / explicit-JSON-null cases (covered
+  // above), not to inputs that aren't JSON at all.
+
+  private static final Object[][] NON_JSON_FIXTURE = {{"value1"}, {"value2"}, {"value3"}, {"value4"}};
+
+  private FluentQueryTest.OnFirstInstance givenNonJsonStringTable(boolean nullHandling) {
+    Schema schema = new Schema.SchemaBuilder()
+        .setSchemaName("notJson")
+        .setEnableColumnBasedNullHandling(true)
+        .addDimensionField("plainString", DataType.STRING)
+        .build();
+    TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName("notJson").build();
+    return FluentQueryTest.withBaseDir(_baseDir)
+        .withNullHandling(nullHandling)
+        .givenTable(schema, tableConfig)
+        .onFirstInstance(NON_JSON_FIXTURE);
+  }
+
+  @Test
+  public void testNonJsonInputThrowsWithNullHandlingOn() {
+    try {
+      givenNonJsonStringTable(true)
+          .whenQuery("SELECT jsonExtractScalar(plainString, '$.path', 'STRING') FROM notJson")
+          .thenResultIs(new Object[]{"unused"});
+      Assert.fail("Expected jsonExtractScalar on non-JSON input to throw even under null handling");
+    } catch (AssertionError e) {
+      Assertions.assertThat(e.getMessage()).contains("not valid JSON");
+    }
+  }
+
+  @Test
+  public void testNonJsonInputThrowsWithNullHandlingOff() {
+    try {
+      givenNonJsonStringTable(false)
+          .whenQuery("SELECT jsonExtractScalar(plainString, '$.path', 'STRING') FROM notJson")
+          .thenResultIs(new Object[]{"unused"});
+      Assert.fail("Expected jsonExtractScalar on non-JSON input to throw with null handling off");
+    } catch (AssertionError e) {
+      Assertions.assertThat(e.getMessage()).contains("not valid JSON");
+    }
+  }
 }
diff --git a/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java b/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java
index 557a6a65fe..68080184c2 100644
--- a/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java
+++ b/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java
@@ -330,10 +330,13 @@ protected Iterator provideTestSqlWithExecutionException() {
     testCases.add(new Object[]{"SELECT jsonExtractKey(col1, 'path') FROM a", "was expecting (JSON String"});
     testCases.add(new Object[]{"SELECT json_extract_key(col1, 'path') FROM a", "was expecting (JSON String"});
 
-    //  - PlaceholderScalarFunction registered will throw on intermediate stage, but works on leaf stage.
-    //    - checked "Illegal Json Path" as col1 is not actually a json string, but the call is correctly triggered.
-    testCases.add(
-        new Object[]{"SELECT CAST(jsonExtractScalar(col1, 'path', 'INT') AS INT) FROM a", "Cannot resolve JSON path"});
+    //  - col1 is a non-JSON STRING column. jsonExtractScalar must reject the input with a clear
+    //    caller-error message rather than silently coercing it to NULL — null handling applies only
+    //    to valid-JSON-with-missing-path, not to input that isn't JSON at all.
+    testCases.add(new Object[]{
+        "SELECT CAST(jsonExtractScalar(col1, 'path', 'INT') AS INT) FROM a",
+        "jsonExtractScalar received input that is not valid JSON"
+    });
     //    - checked function cannot be found b/c there's no intermediate stage impl for json_extract_scalar
     testCases.add(new Object[]{
         "SELECT CAST(json_extract_scalar(a.col1, b.col2, 'INT') AS INT) FROM a JOIN b ON a.col1 = b.col1",

From a383ce349e33510a0a541018279e21feafd408f3 Mon Sep 17 00:00:00 2001
From: Siddharth Teotia 
Date: Fri, 12 Jun 2026 13:43:05 -0700
Subject: [PATCH 14/14] Fix null-input regression in JSON-extract catch
 narrowing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The prior WIP commit narrowed the per-row catch to PathNotFoundException +
InvalidJsonException to surface non-JSON input as a hard error. But jayway
throws a third, distinct exception for a null/empty JSON cell —
IllegalArgumentException("json can not be null") from ParseContextImpl.parse —
which neither arm caught. That exception escaped and broke the 14
testDefaultValue cases from #204 (null JSON cell + a configured default must
return the default, not throw).

Restore the broad catch for everything that represents a missing value, and
peel off only genuinely malformed input:

  - InvalidJsonException        -> throw IllegalArgumentException("...not valid
                                   JSON..."), regardless of null handling
                                   (non-JSON column is a schema modeling error).
  - everything else (null/empty input, missing path) -> swallowed; result stays
                                   null and the existing null-handling / default
                                   cascade decides (preserves #204 and #205).

Add a boundary test (testNonJsonInputThrowsEvenWithDefaultValue) proving a
configured default does not mask non-JSON input — the parse failure is thrown
before the default cascade is reached.

Verified locally: JsonExtractScalarTransformFunctionTest 145/145,
QueryRunnerTest 131/131 (case #12 throws the expected message), spotless +
checkstyle clean on pinot-core and pinot-query-runtime.

Co-Authored-By: Claude Opus 4.8 (1M context) 
---
 .../JsonExtractScalarTransformFunction.java   | 133 ++++++++++--------
 ...sonExtractScalarTransformFunctionTest.java |  16 +++
 2 files changed, 88 insertions(+), 61 deletions(-)

diff --git a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
index 73f7448028..54fd5cbee1 100644
--- a/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
+++ b/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java
@@ -26,7 +26,6 @@
 import com.jayway.jsonpath.JsonPath;
 import com.jayway.jsonpath.Option;
 import com.jayway.jsonpath.ParseContext;
-import com.jayway.jsonpath.PathNotFoundException;
 import com.jayway.jsonpath.spi.json.JacksonJsonProvider;
 import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
 import java.math.BigDecimal;
@@ -218,15 +217,16 @@ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         nullBitmap.add(i);
@@ -255,15 +255,16 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -298,15 +299,16 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -338,15 +340,16 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -378,15 +381,16 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -418,15 +422,16 @@ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -458,15 +463,16 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
       Object result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         if (_defaultValue != null) {
@@ -499,15 +505,16 @@ public int[][] transformToIntValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         _intValuesMV[i] = new int[0];
@@ -547,15 +554,16 @@ public long[][] transformToLongValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         _longValuesMV[i] = new long[0];
@@ -594,15 +602,16 @@ public float[][] transformToFloatValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         _floatValuesMV[i] = new float[0];
@@ -641,15 +650,16 @@ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         _doubleValuesMV[i] = new double[0];
@@ -688,15 +698,16 @@ public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
       List result = null;
       try {
         result = resultExtractor.apply(i);
-      } catch (PathNotFoundException ignored) {
-        // result stays null; the cascade below applies null-handling or the default value.
       } catch (InvalidJsonException e) {
-        // Non-parseable input is a caller / schema modeling error (e.g. jsonExtractScalar
-        // invoked on a non-JSON column), not a missing value — surface it regardless of
-        // _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
+        // Input that is not valid JSON at all (as opposed to valid JSON with a missing path) is a
+        // caller / schema modeling error — e.g. jsonExtractScalar invoked on a non-JSON column.
+        // Surface it regardless of _nullHandlingEnabled so the bug isn't silently masked as SQL NULL.
         throw new IllegalArgumentException(
             "jsonExtractScalar received input that is not valid JSON. "
                 + "This function requires a JSON-typed column or a JSON-formatted string.", e);
+      } catch (Exception ignored) {
+        // Null / empty input or a missing path: result stays null and the cascade below applies
+        // null-handling or the default value (preserves PR #205's behavior).
       }
       if (result == null) {
         _stringValuesMV[i] = new String[0];
diff --git a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
index 9f626bfbd5..0b47dcd2ed 100644
--- a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
+++ b/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunctionTest.java
@@ -1408,4 +1408,20 @@ public void testNonJsonInputThrowsWithNullHandlingOff() {
       Assertions.assertThat(e.getMessage()).contains("not valid JSON");
     }
   }
+
+  // A configured default value substitutes for a *missing* value (valid JSON, absent path, or null
+  // cell — see testDefaultValue). It must NOT mask non-JSON input: parse failure is thrown before the
+  // default cascade is reached, so the default never gets a chance to swallow a schema modeling error.
+  // Null handling on + a default is the most masking-prone config; if it throws here, it throws anywhere.
+  @Test
+  public void testNonJsonInputThrowsEvenWithDefaultValue() {
+    try {
+      givenNonJsonStringTable(true)
+          .whenQuery("SELECT jsonExtractScalar(plainString, '$.path', 'STRING', 'fallback') FROM notJson")
+          .thenResultIs(new Object[]{"unused"});
+      Assert.fail("Expected jsonExtractScalar on non-JSON input to throw even when a default value is set");
+    } catch (AssertionError e) {
+      Assertions.assertThat(e.getMessage()).contains("not valid JSON");
+    }
+  }
 }