cloud-fan commented on code in PR #58024:
URL: https://github.com/apache/spark/pull/58024#discussion_r4385471348
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseLexer.g4:
##########
@@ -80,10 +80,52 @@ import java.util.Deque;
public int complex_type_level_counter = 0;
/**
- * Increase the counter by one when hits KEYWORD 'ARRAY', 'MAP', 'STRUCT'.
+ * Increase the counter by one when hits KEYWORD 'ARRAY', 'MAP', 'STRUCT'
that starts an
+ * angle-bracketed complex type, such as ARRAY<INT>. Other uses of these
keywords, such as the
+ * ARRAY(1, 2) function or the `IS JSON ARRAY` predicate, have no matching
'>' and must leave
+ * the counter unchanged, otherwise a later '>>' could not be lexed as a
shift operator.
*/
public void incComplexTypeLevelCounter() {
- complex_type_level_counter++;
+ if (isFollowedByLessThan()) complex_type_level_counter++;
+ }
+
+ /**
+ * Returns whether the next character after the current token, skipping
whitespace and
+ * comments, is '<'. Called from a lexer action, where the input is
positioned right after the
+ * matched keyword.
+ */
+ private boolean isFollowedByLessThan() {
+ int i = 1;
+ while (true) {
+ int c = _input.LA(i);
+ if (c != CharStream.EOF && (Character.isWhitespace(c) ||
Character.isSpaceChar(c))) {
+ // Covers every character of the WS rule, including no-break spaces.
+ i++;
+ } else if (c == '-' && _input.LA(i + 1) == '-') {
+ // Simple comment: skip to the end of the line, honoring backslash
line continuations.
+ i += 2;
+ while (_input.LA(i) != '\n' && _input.LA(i) != CharStream.EOF) {
Review Comment:
**Blocking (P1):** `isFollowedByLessThan` ends a `--` comment only at LF,
but the lexer's `SIMPLE_COMMENT` rule (line 765) also ends one at a bare CR.
When SQL text uses bare-CR line endings and a `--` comment sits between
`ARRAY`/`MAP`/`STRUCT` and its `<`, this loop runs past the `<` to EOF (or the
next LF), the lookahead returns false, and the keyword is no longer counted.
Once enough of the open keywords go uncounted (both, for a two-level type), the
adjacent closers lex as a shift operator and a type that parses on master
becomes a syntax error. This applies to every CAST target, DDL column type, and
schema string this lexer parses; CRLF and LF line endings are unaffected.
I tokenized `CAST(NULL AS ARRAY -- c<CR><ARRAY -- c<CR><INT>>)` (`<CR>` is a
bare carriage return) with lexers generated by ANTLR 4.13.1 from both
revisions: master gives `LT LT GT GT`, this PR gives `LT LT SHIFT_RIGHT`. The
three-level `>>>` and the MAP forms fail the same way. Also stopping at `'\r'`
matches `SIMPLE_COMMENT`, and with that change every case lexes as on master
while `SELECT array(1), 8 >> 1` and `'[1]' IS JSON ARRAY, 8 >> 1` still get
`SHIFT_RIGHT`:
```suggestion
while (_input.LA(i) != '\n' && _input.LA(i) != '\r' && _input.LA(i)
!= CharStream.EOF) {
```
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala:
##########
@@ -1138,4 +1138,81 @@ class JsonExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
StructType(StructField("f", FileType.external) :: Nil))
}
}
+
+ test("SPARK-58793: IS JSON evaluates its operand exactly once") {
+ // Regression test: a non-deterministic operand (e.g. `IF(rand() < 0.5,
NULL, '{}')`) must be
+ // evaluated a single time, otherwise the predicate could observe two
different values and
+ // return a wrong result (false, when only NULL or true are valid). The
replacement must
+ // therefore reference the operand exactly once.
+ val child = Literal.create("{}", StringType)
+ Seq(IsJsonShape.Any, IsJsonShape.Value, IsJsonShape.Object,
IsJsonShape.Array,
+ IsJsonShape.Scalar).foreach { shape =>
+ val references = IsJson(child, shape).replacement.collect { case e if e
eq child => e }
+ assert(references.length == 1,
+ s"operand referenced ${references.length} times for shape $shape,
expected exactly once")
+ }
+ }
+
+ test("SPARK-58793: IS JSON requires strict ANSI JSON (no Hive leniency)") {
Review Comment:
**Nit (P3):** The strictness tests cover single quotes and raw control
characters, but several false cases that `sql-ref-is-json.md` promises have no
assertion: comments, unquoted field names, `NaN`/`Infinity`, leading zeros, and
whitespace-only input. The documented acceptance of duplicate keys and the
operand contract are untested too: there is no `UTF8_LCASE` or trim-collated
operand, no `VARCHAR`, no untyped `NULL IS JSON` (the PR description's example,
which relies on the `ExpectsInputTypes` NullType coercion), and no case showing
that `1 IS JSON` is rejected. Enabling `ALLOW_NON_NUMERIC_NUMBERS` or
`ALLOW_JAVA_COMMENTS` on the strict factory, turning on duplicate detection, or
switching to `ImplicitCastInputTypes` (which would make `123 IS JSON` true)
would all leave every current test green. A few cases here or in
`json-functions.sql`, with the golden files regenerated, would pin the
documented contract.
**Verification:**
- **Behavior:** Each documented false case (comments, unquoted field names,
NaN or Infinity, leading zeros, whitespace-only input) makes IS JSON return
false, and an object with duplicate field names makes it return true.
- **Behavior:** IS JSON over a UTF8_LCASE operand, a trim-collated operand,
and a VARCHAR operand returns the same results as over a plain STRING; untyped
`NULL IS JSON` returns NULL; and `1 IS JSON` fails analysis with a data-type
mismatch.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala:
##########
@@ -291,6 +291,31 @@ class ExpressionParserSuite extends AnalysisTest {
assertEqual("a is not distinct from b", $"a" <=> $"b")
}
+ test("SPARK-58793: is json expressions") {
+ assertEqual("a is json", IsJson($"a", IsJsonShape.Any))
+ assertEqual("a is not json", !IsJson($"a", IsJsonShape.Any))
+ assertEqual("a is json value", IsJson($"a", IsJsonShape.Value))
+ assertEqual("a is json object", IsJson($"a", IsJsonShape.Object))
+ assertEqual("a is not json array", !IsJson($"a", IsJsonShape.Array))
+ assertEqual("a is json scalar", IsJson($"a", IsJsonShape.Scalar))
+ // A non-type ARRAY/MAP/STRUCT keyword must not leave the lexer in
complex-type mode, which
+ // would make a later `>>` or `>>>` lex as two `>` tokens and fail to
parse.
+ assertEqual("(a is json array) = (b >> 1)",
+ IsJson($"a", IsJsonShape.Array) === ShiftRight($"b", 1))
+ assertEqual("(a is json array) = (b >>> 1)",
+ IsJson($"a", IsJsonShape.Array) === ShiftRightUnsigned($"b", 1))
+ assertEqual("array(a) = array(b >> 1)",
+ $"array".function($"a") === $"array".function(ShiftRight($"b", 1)))
+ assertEqual("map(a, b >> 1)", $"map".function($"a", ShiftRight($"b", 1)))
+ assertEqual("struct(a, b >>> 1)", CreateStruct(Seq($"a",
ShiftRightUnsigned($"b", 1))))
+ // Angle-bracketed complex types still split consecutive closing brackets,
including when
+ // whitespace or comments separate the keyword from '<'.
+ assertEqual("cast(a as array<array<int>>) = b >> 1",
+ Cast($"a", ArrayType(ArrayType(IntegerType))) === ShiftRight($"b", 1))
+ assertEqual("cast(a as map /* c */ <int, array -- c\n <int>>) = b >> 1",
Review Comment:
**Nit (P3):** These comment cases can't catch a regression in a single
comment-skipping branch of `isFollowedByLessThan`. Here `map` has a bracketed
comment and `array` a `--` comment, so if either branch alone breaks, the other
keyword is still counted and `>>` still splits; the case fails only when both
branches break. The new `bitwise.sql` case comments only the outer `array`, so
it passes even with no comment skipping at all. I checked both with lexers
generated from this grammar with one or both branches forced to return false.
To isolate a branch, the same comment form has to precede every type keyword of
the nested type, e.g. `array /* c */ <array /* c */ <int>>` for `>>` and a
three-level type for `>>>`. A comment before only the inner keyword doesn't
work either, because the outer keyword is still counted directly. Bare-CR,
CRLF, nested bracketed, and backslash-continued `--` comments have no case yet;
the bare-CR form is the one that currently breaks.
This extends an [existing
discussion](https://github.com/apache/spark/pull/58024#discussion_r4068683249).
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionUtils.java:
##########
@@ -117,4 +118,62 @@ public static UTF8String jsonTypeof(UTF8String json) {
return null;
}
}
+
+ // Shape codes for the SQL `IS [NOT] JSON` predicate, kept in sync with
IsJsonShape on the
+ // Scala side. SHAPE_ANY covers both bare `IS JSON` and `IS JSON VALUE` (any
well-formed value).
+ public static final int SHAPE_ANY = 0;
+ public static final int SHAPE_OBJECT = 1;
+ public static final int SHAPE_ARRAY = 2;
+ public static final int SHAPE_SCALAR = 3;
+
+ // A strict JSON factory for the ANSI SQL `IS [NOT] JSON` predicate. Unlike
the shared factory
+ // used by the Hive-compatible JSON functions, it leaves ALLOW_SINGLE_QUOTES
and
+ // ALLOW_UNESCAPED_CONTROL_CHARS disabled, so only well-formed ANSI JSON is
accepted
+ // (e.g. `{'a':1}` with single quotes is rejected).
+ private static final JsonFactory STRICT_JSON_FACTORY = new JsonFactory();
+
+ /**
+ * Evaluates the SQL `IS JSON` predicate for a single, already-evaluated
operand: returns whether
+ * {@code json} is a well-formed ANSI JSON value of the requested {@code
shape}. The operand is
+ * read exactly once here, so a non-deterministic input is evaluated a
single time (unlike a
+ * plan that references the child in more than one place). NULL handling
(NULL input yields a
+ * NULL result) is left to the calling expression; this method is only
invoked for non-null
+ * input and always returns a definite true/false.
+ */
+ public static boolean isJson(UTF8String json, int shape) {
+ try (JsonParser jsonParser =
+ CreateJacksonParser.utf8String(STRICT_JSON_FACTORY, json)) {
Review Comment:
Thanks, verified. `isJson` now returns false when `UTF8String.isValid()`
fails, before decoding, and the `X'228022'` and `X'22EDA08022'` cases cover it.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4068683245","thread_id":"inline:4068683245","verdict_sha256":"38cf435f3ba8bf60d83b17dd355dd38452a4bcf26c8a70b16e14d3ea33ac3f9b"}
-->
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -1417,6 +1417,7 @@ predicate
| IS errorCapturingNot? kind=NULL
| IS errorCapturingNot? kind=(TRUE | FALSE | UNKNOWN)
| IS errorCapturingNot? kind=DISTINCT FROM right=valueExpression
+ | IS errorCapturingNot? kind=JSON shape=(VALUE | ARRAY | OBJECT | SCALAR)?
Review Comment:
Thanks, the counter now moves only when the keyword is followed by `<`, and
the `IS JSON ARRAY` and `array(...)` cases followed by `>>`/`>>>` parse. This
review also raises a testing follow-up: the lookahead's comment cases don't
fail if only one of its comment-skipping branches regresses.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4068683249","thread_id":"inline:4068683249","verdict_sha256":"38cf435f3ba8bf60d83b17dd355dd38452a4bcf26c8a70b16e14d3ea33ac3f9b"}
-->
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionUtils.java:
##########
@@ -117,4 +118,62 @@ public static UTF8String jsonTypeof(UTF8String json) {
return null;
}
}
+
+ // Shape codes for the SQL `IS [NOT] JSON` predicate, kept in sync with
IsJsonShape on the
+ // Scala side. SHAPE_ANY covers both bare `IS JSON` and `IS JSON VALUE` (any
well-formed value).
+ public static final int SHAPE_ANY = 0;
+ public static final int SHAPE_OBJECT = 1;
+ public static final int SHAPE_ARRAY = 2;
+ public static final int SHAPE_SCALAR = 3;
+
+ // A strict JSON factory for the ANSI SQL `IS [NOT] JSON` predicate. Unlike
the shared factory
+ // used by the Hive-compatible JSON functions, it leaves ALLOW_SINGLE_QUOTES
and
+ // ALLOW_UNESCAPED_CONTROL_CHARS disabled, so only well-formed ANSI JSON is
accepted
+ // (e.g. `{'a':1}` with single quotes is rejected).
+ private static final JsonFactory STRICT_JSON_FACTORY = new JsonFactory();
+
+ /**
+ * Evaluates the SQL `IS JSON` predicate for a single, already-evaluated
operand: returns whether
+ * {@code json} is a well-formed ANSI JSON value of the requested {@code
shape}. The operand is
+ * read exactly once here, so a non-deterministic input is evaluated a
single time (unlike a
+ * plan that references the child in more than one place). NULL handling
(NULL input yields a
+ * NULL result) is left to the calling expression; this method is only
invoked for non-null
+ * input and always returns a definite true/false.
+ */
+ public static boolean isJson(UTF8String json, int shape) {
+ try (JsonParser jsonParser =
+ CreateJacksonParser.utf8String(STRICT_JSON_FACTORY, json)) {
+ JsonToken token = jsonParser.nextToken();
Review Comment:
Thanks, verified. The full token walk now rejects unpaired surrogate escapes
in both values and field names at any depth, and correctly paired escapes still
pass.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4068683254","thread_id":"inline:4068683254","verdict_sha256":"38cf435f3ba8bf60d83b17dd355dd38452a4bcf26c8a70b16e14d3ea33ac3f9b"}
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
