Making JSON datasource respectful to spark.sql.caseSensitive by MaxGekk · Pull Request #3 · MaxGekk/spark

MaxGekk · 2018-04-14T18:18:26Z

What changes were proposed in this pull request?

Currently, JSON datasource is case sensitive and not respectful to the caseSensitive parameter. It causes the problem of reading json records with fields in different cases. For example, the following json file - test.json contains fields in different cases:

{"field": "a"}
{"Field": "b"}
{"FIELD": "c"}

Creating the table test by using the json file:

CREATE TABLE test USING json LOCATION test.json

Querying the table:

SELECT field FROM test

returns only the first row:

+-----+
|field|
+-----+
|    a|
| null|
| null|
+-----+

After applying the proposed changes:

+-----+
|field|
+-----+
|    a|
|    b|
|    c|
+-----+

How was this patch tested?

### What changes were proposed in this pull request? `org.apache.spark.sql.kafka010.KafkaDelegationTokenSuite` failed lately. After had a look at the logs it just shows the following fact without any details: ``` Caused by: sbt.ForkMain$ForkError: sun.security.krb5.KrbException: Server not found in Kerberos database (7) - Server not found in Kerberos database ``` Since the issue is intermittent and not able to reproduce it we should add more debug information and wait for reproduction with the extended logs. ### Why are the changes needed? Failing test doesn't give enough debug information. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? I've started the test manually and checked that such additional debug messages show up: ``` >>> KrbApReq: APOptions are 00000000 00000000 00000000 00000000 >>> EType: sun.security.krb5.internal.crypto.Aes128CtsHmacSha1EType Looking for keys for: kafka/localhostEXAMPLE.COM Added key: 17version: 0 Added key: 23version: 0 Added key: 16version: 0 Found unsupported keytype (3) for kafka/localhostEXAMPLE.COM >>> EType: sun.security.krb5.internal.crypto.Aes128CtsHmacSha1EType Using builtin default etypes for permitted_enctypes default etypes for permitted_enctypes: 17 16 23. >>> EType: sun.security.krb5.internal.crypto.Aes128CtsHmacSha1EType MemoryCache: add 1571936500/174770/16C565221B70AAB2BEFE31A83D13A2F4/client/localhostEXAMPLE.COM to client/localhostEXAMPLE.COM|kafka/localhostEXAMPLE.COM MemoryCache: Existing AuthList: #3: 1571936493/200803/8CD70D280B0862C5DA1FF901ECAD39FE/client/localhostEXAMPLE.COM #2: 1571936499/985009/BAD33290D079DD4E3579A8686EC326B7/client/localhostEXAMPLE.COM #1: 1571936499/995208/B76B9D78A9BE283AC78340157107FD40/client/localhostEXAMPLE.COM ``` Closes apache#26252 from gaborgsomogyi/SPARK-29580. Authored-by: Gabor Somogyi <gabor.g.somogyi@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

### What changes were proposed in this pull request? This PR proposes to make `PythonFunction` holds `Seq[Byte]` instead of `Array[Byte]` to be able to compare if the byte array has the same values for the cache manager. ### Why are the changes needed? Currently the cache manager doesn't use the cache for `udf` if the `udf` is created again even if the functions is the same. ```py >>> func = lambda x: x >>> df = spark.range(1) >>> df.select(udf(func)("id")).cache() ``` ```py >>> df.select(udf(func)("id")).explain() == Physical Plan == *(2) Project [pythonUDF0#14 AS <lambda>(id)#12] +- BatchEvalPython [<lambda>(id#0L)], [pythonUDF0#14] +- *(1) Range (0, 1, step=1, splits=12) ``` This is because `PythonFunction` holds `Array[Byte]`, and `equals` method of array equals only when the both array is the same instance. ### Does this PR introduce _any_ user-facing change? Yes, if the user reuse the Python function for the UDF, the cache manager will detect the same function and use the cache for it. ### How was this patch tested? I added a test case and manually. ```py >>> df.select(udf(func)("id")).explain() == Physical Plan == InMemoryTableScan [<lambda>(id)#12] +- InMemoryRelation [<lambda>(id)#12], StorageLevel(disk, memory, deserialized, 1 replicas) +- *(2) Project [pythonUDF0#5 AS <lambda>(id)#3] +- BatchEvalPython [<lambda>(id#0L)], [pythonUDF0#5] +- *(1) Range (0, 1, step=1, splits=12) ``` Closes apache#28774 from ueshin/issues/SPARK-31945/udf_cache. Authored-by: Takuya UESHIN <ueshin@databricks.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

… without WindowExpression ### What changes were proposed in this pull request? Add WindowFunction check at `CheckAnalysis`. ### Why are the changes needed? Provide friendly error msg. **BEFORE** ```scala scala> sql("select rank() from values(1)").show java.lang.UnsupportedOperationException: Cannot generate code for expression: rank() ``` **AFTER** ```scala scala> sql("select rank() from values(1)").show org.apache.spark.sql.AnalysisException: Window function rank() requires an OVER clause.;; Project [rank() AS RANK()#3] +- LocalRelation [col1#2] ``` ### Does this PR introduce _any_ user-facing change? Yes, user wiill be given a better error msg. ### How was this patch tested? Pass the newly added UT. Closes apache#28808 from ulysses-you/SPARK-31975. Authored-by: ulysses <youxiduo@weidian.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ly equivalent children in `RewriteDistinctAggregates` ### What changes were proposed in this pull request? In `RewriteDistinctAggregates`, when grouping aggregate expressions by function children, treat children that are semantically equivalent as the same. ### Why are the changes needed? This PR will reduce the number of projections in the Expand operator when there are multiple distinct aggregations with superficially different children. In some cases, it will eliminate the need for an Expand operator. Example: In the following query, the Expand operator creates 3\*n rows (where n is the number of incoming rows) because it has a projection for each of function children `b + 1`, `1 + b` and `c`. ``` create or replace temp view v1 as select * from values (1, 2, 3.0), (1, 3, 4.0), (2, 4, 2.5), (2, 3, 1.0) v1(a, b, c); select a, count(distinct b + 1), avg(distinct 1 + b) filter (where c > 0), sum(c) from v1 group by a; ``` The Expand operator has three projections (each producing a row for each incoming row): ``` [a#87, null, null, 0, null, UnscaledValue(c#89)], <== projection #1 (for regular aggregation) [a#87, (b#88 + 1), null, 1, null, null], <== projection #2 (for distinct aggregation of b + 1) [a#87, null, (1 + b#88), 2, (c#89 > 0.0), null]], <== projection #3 (for distinct aggregation of 1 + b) ``` In reality, the Expand only needs one projection for `1 + b` and `b + 1`, because they are semantically equivalent. With the proposed change, the Expand operator's projections look like this: ``` [a#67, null, 0, null, UnscaledValue(c#69)], <== projection #1 (for regular aggregations) [a#67, (b#68 + 1), 1, (c#69 > 0.0), null]], <== projection #2 (for distinct aggregation on b + 1 and 1 + b) ``` With one less projection, Expand produces 2\*n rows instead of 3\*n rows, but still produces the correct result. In the case where all distinct aggregates have semantically equivalent children, the Expand operator is not needed at all. Benchmark code in the JIRA (SPARK-40382). Before the PR: ``` distinct aggregates: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ all semantically equivalent 14721 14859 195 5.7 175.5 1.0X some semantically equivalent 14569 14572 5 5.8 173.7 1.0X none semantically equivalent 14408 14488 113 5.8 171.8 1.0X ``` After the PR: ``` distinct aggregates: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ all semantically equivalent 3658 3692 49 22.9 43.6 1.0X some semantically equivalent 9124 9214 127 9.2 108.8 0.4X none semantically equivalent 14601 14777 250 5.7 174.1 0.3X ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit tests. Closes apache#37825 from bersprockets/rewritedistinct_issue. Authored-by: Bruce Robbins <bersprockets@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…edExpression() ### What changes were proposed in this pull request? In `EquivalentExpressions.addExpr()`, add a guard `supportedExpression()` to make it consistent with `addExprTree()` and `getExprState()`. ### Why are the changes needed? This fixes a regression caused by apache#39010 which added the `supportedExpression()` to `addExprTree()` and `getExprState()` but not `addExpr()`. One example of a use case affected by the inconsistency is the `PhysicalAggregation` pattern in physical planning. There, it calls `addExpr()` to deduplicate the aggregate expressions, and then calls `getExprState()` to deduplicate the result expressions. Guarding inconsistently will cause the aggregate and result expressions go out of sync, eventually resulting in query execution error (or whole-stage codegen error). ### Does this PR introduce _any_ user-facing change? This fixes a regression affecting Spark 3.3.2+, where it may manifest as an error running aggregate operators with higher-order functions. Example running the SQL command: ```sql select max(transform(array(id), x -> x)), max(transform(array(id), x -> x)) from range(2) ``` example error message before the fix: ``` java.lang.IllegalStateException: Couldn't find max(transform(array(id#0L), lambdafunction(lambda x#2L, lambda x#2L, false)))#4 in [max(transform(array(id#0L), lambdafunction(lambda x#1L, lambda x#1L, false)))#3] ``` after the fix this error is gone. ### How was this patch tested? Added new test cases to `SubexpressionEliminationSuite` for the immediate issue, and to `DataFrameAggregateSuite` for an example of user-visible symptom. Closes apache#40473 from rednaxelafx/spark-42851. Authored-by: Kris Mok <kris.mok@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…edExpression() ### What changes were proposed in this pull request? In `EquivalentExpressions.addExpr()`, add a guard `supportedExpression()` to make it consistent with `addExprTree()` and `getExprState()`. ### Why are the changes needed? This fixes a regression caused by apache#39010 which added the `supportedExpression()` to `addExprTree()` and `getExprState()` but not `addExpr()`. One example of a use case affected by the inconsistency is the `PhysicalAggregation` pattern in physical planning. There, it calls `addExpr()` to deduplicate the aggregate expressions, and then calls `getExprState()` to deduplicate the result expressions. Guarding inconsistently will cause the aggregate and result expressions go out of sync, eventually resulting in query execution error (or whole-stage codegen error). ### Does this PR introduce _any_ user-facing change? This fixes a regression affecting Spark 3.3.2+, where it may manifest as an error running aggregate operators with higher-order functions. Example running the SQL command: ```sql select max(transform(array(id), x -> x)), max(transform(array(id), x -> x)) from range(2) ``` example error message before the fix: ``` java.lang.IllegalStateException: Couldn't find max(transform(array(id#0L), lambdafunction(lambda x#2L, lambda x#2L, false)))#4 in [max(transform(array(id#0L), lambdafunction(lambda x#1L, lambda x#1L, false)))#3] ``` after the fix this error is gone. ### How was this patch tested? Added new test cases to `SubexpressionEliminationSuite` for the immediate issue, and to `DataFrameAggregateSuite` for an example of user-visible symptom. Closes apache#40473 from rednaxelafx/spark-42851. Authored-by: Kris Mok <kris.mok@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit ef0a76e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…framework ### What changes were proposed in this pull request? Add a `--DEBUG` marker directive for the SQL golden file test framework (`SQLQueryTestSuite`). When placed on its own line before a query in an input `.sql` file, it enables a focused debug mode: - **Selective execution**: Only `--DEBUG`-marked queries and setup commands (CREATE TABLE, INSERT, SET, etc.) are executed; all other queries are skipped. - **Full error details**: Failed queries print the complete stacktrace to the console. - **Golden comparison**: Results are still compared against the golden file so you can verify correctness. - **Safety net**: The test always fails at the end with a reminder to remove `--DEBUG` markers before committing. - **DataFrame access**: Documentation guides users to set a breakpoint in `runDebugQueries` to inspect the `DataFrame` instance for ad-hoc plan analysis. Example usage in an input file: ```sql CREATE TABLE t (id INT, val INT) USING parquet; INSERT INTO t VALUES (1, 10), (2, 20); -- this query is skipped in debug mode SELECT count(*) FROM t; -- this is the query I'm debugging --DEBUG SELECT sum(val) OVER (ORDER BY id) FROM t; ``` Example console output when running the test: ``` === DEBUG: Query #3 === SQL: SELECT sum(val) OVER (ORDER BY id) FROM t Golden answer: matches ``` When the debug query fails: ``` === DEBUG: Query #3 === SQL: SELECT sum(val) OVER (ORDER BY id) FROM t org.apache.spark.sql.AnalysisException: [ERROR_CLASS] ... at org.apache.spark.sql.catalyst.analysis... at ... Golden answer: matches ``` ### Why are the changes needed? Debugging golden file test failures is currently painful: 1. You must run all queries even if only one needs debugging. 2. Error output is minimal (just the error class/message), with no stacktrace. 3. There is no way to access the `DataFrame` instance for plan inspection. This change addresses all three issues with a simple, zero-config marker that can be temporarily added during development. ### Does this PR introduce _any_ user-facing change? No. This is a test infrastructure improvement only. ### How was this patch tested? - Manually tested with `--DEBUG` markers on both passing and failing queries in `inline-table.sql`. - Verified backward compatibility: tests pass normally when no `--DEBUG` markers are present. - Verified debug mode output includes full stacktraces, golden answer comparison, and the safety-fail message. ### Was this patch authored or co-authored using generative AI tooling? Yes. cursor Closes apache#54554 from cloud-fan/golden. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Kent Yao <kentyao@microsoft.com>

… metadata ### What changes were proposed in this pull request? Follow-up to apache#55636 addressing post-merge review comments from zikangh: 1. **Deduplicate `isCarryoverPair`.** The carry-over predicate (`_del_cnt = 1 AND _ins_cnt = 1 AND _rv_cnt = 2 AND _min_rv = _max_rv`) was duplicated between the batch path's `addCarryOverPairFilter` and the streaming path's inline filter. Extracted a shared `buildCarryOverPairPredicate` helper and call it from both. 2. **Mark the streaming row-level rewrite via attribute metadata rather than helper column name.** `UnsupportedOperationChecker` previously detected the rewrite by string-matching the `__spark_cdc_events` aggregate alias name. Switched to a metadata marker (`ResolveChangelogTable.streamingPostProcessingMarker`) attached to the alias's output attribute -- mirroring the existing `EventTimeWatermark.delayKey` and `SessionWindow.marker` patterns. The marker travels with the attribute through optimization. 3. **Expand streaming E2E coverage.** New tests in `ChangelogEndToEndSuite`: - composite rowId carry-over removal, - composite rowId update detection (different tuples kept raw), - carry-over + update detection across multiple commits, - DELETE-all-rows and UPDATE-all-rows fixtures, - append-only workload pass-through, - no-op UPDATE labeled as update (rcv differs on pre/post), - large carry-over removal (9 carry-over pairs + 1 real delete). ### Why are the changes needed? zikangh raised these on the merged PR. Bundled together so they can be reviewed and shipped as one follow-up. ### Does this PR introduce _any_ user-facing change? No. Internal refactor (#1, #2) and additional test coverage (#3). The behavior of streaming CDC reads is unchanged. ### How was this patch tested? All 157 tests pass across the four CDC suites: - `ChangelogResolutionSuite` - `ResolveChangelogTablePostProcessingSuite` - `ResolveChangelogTableStreamingPostProcessingSuite` - `ChangelogEndToEndSuite` Also confirmed: - `UnsupportedOperationsSuite` (216 tests) still passes after the marker-based detection switch. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (claude-opus-4-7) Closes apache#55653 from gengliangwang/streamingCDC-followup-zikangh. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? Address the open follow-ups from [SPARK-56681](https://issues.apache.org/jira/browse/SPARK-56681) (umbrella for PATH / SPARK-56605 cleanup) in a single cleanup PR. Items #1 and #2 were already wired by SPARK-56639; this PR covers the remainder. | # | Item | Resolution | |---|---|---| | #1 | `FunctionResolution.resolveProcedure` was dead code | Already wired by SPARK-56639 (no action). | | #2 | Frozen view / SQL-function PATH wiring unfinished | Already done by SPARK-56639 (no action). | | #3 | `AnalysisContext.resolutionPathEntries` threadlocal | Audit only: confirmed `withNewAnalysisContext` / `reset()` correctly clear it. Full removal needs a coordinated refactor to plumb the path through `RelationResolution` / `FunctionResolution` method calls; flagged as a follow-up. | | #4 | `Analyzer.executeAndCheck` clobbers outer `SQLConf.withExistingConf` | Extracted `runWithSessionConf` helper, added `SQLConf.getExistingConfIfSet`. `executeAndCheck` and `executeSameContext` now share one path that yields to any outer scope. | | #5 | `VariableResolution.allowUnqualifiedSessionTempVariableLookup` force-loads default catalog | Replaced the hot-path catalog read with `CatalogManager.isSystemSessionOnPath`, which inspects stored session-path entries directly. No catalog load on column resolution. | | #6 | `DROP VARIABLE` PATH gate asymmetric with `DECLARE` / `CREATE` | Removed the gate. DDL on session variables (`DECLARE` / `CREATE` / `DROP`) always targets `system.session` directly; only DML (`SET VAR`, `SELECT x`) goes through PATH. | | #7 | `lookupFunctionType` exception swallow too broad | Narrowed from `NonFatal` to the explicit not-found list (`NoSuchFunctionException`, `NoSuchNamespaceException`, `CatalogNotFoundException`, `FORBIDDEN_OPERATION`). Other exceptions propagate. | | #8 | `lookupFunctionType` fan-out had wasteful `system.*` candidates | Filtered them out — `system.session`, `system.builtin`, `system.ai` are already resolved earlier in the same method. | | #9 | Three near-duplicate path-resolution helpers | Lifted into `CatalogManager.resolutionPathEntriesForAnalysis(pinnedEntries, viewCatalogAndNamespace)`. Relation, routine, and procedure resolution all route through it. | | #10 | Tests for the new error paths and gates | Added a DECLARE / SET VAR / DROP cycle test under non-default PATH and a struct-variable field-vs-qualified ambiguity test in `sql-session-variables.sql`. | | #11 | `ProtoToParsedPlanTestSuite.analyzerIsolationConf` was a bare `SQLConf` | Clone `spark.sessionState.conf` and only override `PATH_ENABLED=false`, so all `sparkConf` overrides (ANSI, alias config, ...) propagate automatically. | | Bonus | `ResolveSetVariable` hardcoded `SYSTEM.SESSION` regardless of actual PATH | `unresolvedVariableError` now takes `Seq[Seq[String]]` path entries with **required** `Origin` (no overloads). DML lookup failures (`SET VAR`, `FETCH ... INTO`) report the full SQL path as a bracketed list, byte-for-byte consistent with `UNRESOLVED_ROUTINE` and `TABLE_OR_VIEW_NOT_FOUND`. DDL name validation in `ResolveCatalogs` continues to report `[system.session]` since PATH does not apply there. Origin is plumbed through `VariableManager.set` so all error sites carry a `queryContext` pointing at the offending variable identifier (parser opt-ins via `withOrigin(identifierReference)` so the highlight is the variable name, not the whole statement). | ### Why are the changes needed? These are the cleanup items called out on SPARK-56681 from the post-merge source review of SPARK-56605. They eliminate dead code paths, plug user-visible bugs (force-loading a misconfigured default catalog on column resolution; clobbering pinned session configs; swallowing real catalog errors as `UNRESOLVED_ROUTINE`), remove the asymmetry between DDL and DML on session variables, and make `UNRESOLVED_VARIABLE` self-consistent with the other "not found" errors. ### Does this PR introduce _any_ user-facing change? Yes. - **`UNRESOLVED_VARIABLE.searchPath`** is now rendered as a bracketed list. For DML lookups (`SET VAR`, `FETCH ... INTO`), the list reflects the actual SQL PATH that was consulted instead of a hardcoded `SYSTEM.SESSION`. For DDL name validation (`DECLARE` / `DROP` with a non-session namespace), the list is `[`` `system`.`session` ``]` since PATH does not apply. - **`UNRESOLVED_VARIABLE`** now always carries a `queryContext` that highlights just the offending variable identifier (e.g. `"builtin.var1"`, `"ses.var1"`), not the whole `DECLARE` / `SET VAR` statement. - **`DROP TEMPORARY VARIABLE`** no longer raises `UNRESOLVED_VARIABLE` when the SQL PATH does not contain `system.session`. DDL on session variables ignores PATH, matching the existing behaviour of `DECLARE OR REPLACE VARIABLE`. - **`lookupFunctionType`** no longer swallows non–`NotFound` errors. A catalog reporting `PERMISSION_DENIED` (or similar) for a function lookup now propagates instead of silently producing `UNRESOLVED_ROUTINE`. ### How was this patch tested? - Added `sql-session-variables.sql` regression test for the struct-variable field-vs-qualified ambiguity (`DECLARE VARIABLE session STRUCT<a INT>` → `SELECT session.a` succeeds → `DROP` → `SELECT session.a` falls through to `UNRESOLVED_COLUMN`). - Updated `SetPathSuite`: DECLARE / SET VAR / DROP cycle under a non-default PATH; bonus test asserts the actual rendered search path and the variable-identifier `queryContext`. - Updated `SqlScriptingExecutionSuite` for the new bracketed `searchPath` and identifier-pinned `queryContext`. - Regenerated `sql-session-variables.sql.out` for the new error shape. - Added `resolutionPathEntriesForAnalysis` stubs to mocked `CatalogManager` instances in `PlanResolutionSuite`, `AlignAssignmentsSuiteBase`, and `TableLookupCacheSuite`. - Ran focused suites locally; all pass: - `build/sbt 'sql/testOnly *SetPathSuite *SqlScriptingExecutionSuite *ExecuteImmediateEndToEndSuite'` - `build/sbt 'sql/testOnly *SimpleSQLViewSuite *SQLFunctionSuite'` - `build/sbt 'sql/testOnly *PlanResolutionSuite *UpdateTableAlignAssignmentsSuite *MergeIntoTableAlignAssignmentsSuite'` - `build/sbt 'catalyst/testOnly *TableLookupCacheSuite *AnalysisSuite *AnalysisErrorSuite *LookupFunctionsSuite'` - `build/sbt 'sql/testOnly *FunctionQualificationSuite *RelationQualificationSuite *DataSourceV2FunctionSuite'` - `build/sbt 'sql/testOnly *SQLQuerySuite'` - `build/sbt 'connect/testOnly *ProtoToParsedPlanTestSuite'` - `build/sbt 'sql/testOnly *SQLQueryTestSuite -- -z sql-session-variables.sql'` - Full `org.apache.spark.sql.catalyst.analysis.*`, `org.apache.spark.sql.catalyst.parser.*`, and `org.apache.spark.sql.analysis.resolver.*` suites. - `scalastyle` and `scalafmt` clean across catalyst, sql, and connect modules. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Cursor Claude Opus 4.7 Closes apache#55647 from srielau/SPARK-56681-patch-clean-up. Authored-by: Serge Rielau <serge@rielau.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

…ng TODO - Add a co-located regression test in TimestampNanosParseSuite asserting that the shared parseTimestampString edits (segments array growth + pinned loop bound) do not change the existing microsecond results of stringToTimestamp / stringToTimestampWithoutTimeZone for >6-digit, <6-digit, exactly-6-digit and 10+-digit fractions (finding #4). - Add a TODO(SPARK-57032) on stringToTimestampNTZNanosAnsi noting that a future user-facing TIMESTAMP_NTZ(p) cast must choose allowTimeZone explicitly instead of relying on the true default (finding #5). - Rename the out-of-range-precision test now that the duplicate truncateNanosWithinMicro helper has been removed; the guard lives in the parse entry points. Findings #1/#2/#3 (helper duplication and the sub-micro side-channel) were resolved by the preceding rebase + reuse commit.

…d of assuming it (apache#874) ### What changes were proposed in this pull request? This fixes two wrong answers that came from one unchecked assumption: that every date column holds years 0001 to 9999. **The bug.** Varka's range analysis treated a bare date column's values as inside 0001-9999 (`VarkaRangeAnalysis.CONTRACT`) and never checked it. Spark stores any int32 day count. Two decisions rested on the assumption: * **Calendar fields fused with no guard.** Past day 14,766,813 (42400-02-29), or before day -5,394,572, the calendar decomposition's first division gets a dividend outside the range its proof covers. A cached table holding such days got wrong answers from a kernel that served the batch: * day 14,766,814 gave day-of-month 2 where Spark gives 1; * day 100,000,000 gave year 69,873 and month 31,848 where Spark gives 275,760 and 9; * `year`, `month`, `dayofmonth`, `dayofyear`, `quarter`, `weekofyear`, `last_day` and `trunc(d, 'MM')` were all affected, as was a column plus a literal. * **An overflow check was dropped.** Under ANSI mode, `datediff(d, d2) * 100` and `-datediff(...)` fused with their overflow check removed, so with a far day the kernel returned a wrapped value where Spark raises `ARITHMETIC_OVERFLOW`. **The fix: check the assumption inside the kernel.** The compiler already had an in-kernel range check, `GuardedRange`, an IR node that passes a value through and makes the kernel decline the batch when a lane is outside `[lo, hi]` (built for `TIME + INTERVAL`). * `VarkaRangeAnalysis.range` now reports every date column whose 0001-9999 assumption its answer rests on. A column under a guarded day producer or a column-offset shift isn't reported, since those are already checked. * Where the compiler acts on such an answer (calendar admission in `VarkaChronoCompiler`, overflow-check removal in `VarkaNodeCompiler.boundedByIntMax`), it notes the column. When the entry finishes compiling, every use of a noted column is wrapped in `GuardedRange(column, 0001-01-01, 9999-12-31)`. This happens before the size budgets look at the tree, so they price what is emitted. * A batch with a day outside 0001-9999 makes the kernel return a decline status. The evaluator then throws the kernel's outputs away and runs the whole batch on Spark's row engine. That is the contract every in-kernel guard already has, so nothing a declined kernel wrote is ever read. * **How the check is emitted.** This column-bound guard (and no other range guard) is emitted cheaply: * In a loop method it is a lane-wise `min` and `max` of the loaded vector into two accumulators, which start at the bounds. That is two vector ops per lane group, with no mask and no branch. After the loop the accumulators are compared against the bounds once, and the result joins the status the method already returns. * The single-pass epilogue, and a guard under an `IF`/`CASE` arm (which must be ANDed with the arm's condition), keep two compares. * **What the check reads.** The column-bound guard reads the column's stored days, null rows included, and not its validity bitmap. On AVX2, turning the bitmap into a lane mask for every lane group costs nine instructions, which is more than a cheap kernel's own work. Arrow zeroes the value buffer when it allocates, reallocates or resets a vector, and setting a null only clears the bit, so a null written by Spark's Arrow writer holds day 0, inside the range. A far day stored under a null row can only come from a producer that computed one there, for example a cached Varka output. Such a batch now declines and is answered by the row engine: slower, never wrong. * Every compile-time decision is unchanged; the assumption just becomes a check. The evaluator's separate per-batch pass (`IntRangeOps.allWithin`) is left only for the `CAST(i AS INTERVAL DAY)` bound. The emitted bytes change only for the 24 bounded coverage rows; no fuzz shape and no price in the emit cost tables moves, and no shape gains or loses a method. Per method, at 512 bits: | shape | dense loop | masked loop | |---|---|---| | `year(d)` | 409 → 494 | 409 → 494 | | `weekofyear(d)` | 622 → 707 | 622 → 707 | | `extract(YEAROFWEEK FROM d)` | 531 → 616 | 531 → 616 | | `year(d) * 100 + month(d)` | 472 → 557 | 472 → 557 | | `datediff(d, d2) + 1` | 167 → 270 | 167 → 270 | The masked loops are now the same bytes as the dense ones: neither reads input validity. * **Tests for the new form.** `VarkaGuardEdgeSuite` drives the column bound in both bodies, in a full lane group and the tail, with the masked body forced, and in a split method. The kernel test harness now stores the bound's two ends under a null of a column read through the column bound, where production stores day 0; it used to store int32's ends, which this guard now reads by design. The guard verdict reads a null row's stored day for this guard. **Why not make the calendar exact over all int32 days?** That was built before as a total lowering, measured 14 to 24% slower, and removed. A per-shape bound that would keep serving the years 10000 to 42400 is recorded as later scope (`m9/SCOPE.md` item 102). ### Why are the changes needed? Varka promises that a failure degrades to Spark's row engine and never changes an answer. A wrong answer is the one failure that fallback can't catch. These two were found by proving the kernels against the date and time specification (apache#868) and confirmed end to end on a cached table. ### Does this PR introduce _any_ user-facing change? Yes, for the better. Queries over date columns holding days outside 0001-9999 now return Spark's answers, because those batches run on the row engine instead of a kernel. Days within 0001-9999 are unaffected. ### How was this patch tested? * **`VarkaFarDaySuite`** (new, 5 tests) compares both engines: * on the reproduction, with and without nulls; * on every `coverage.json` row over a date column, past the bound; * at the bound's edges; * on a column plus a literal, and on filters; * on the `datediff` overflow case. Which rows must decline is derived from the coverage domains: 24 of the 51 rows over a date column are bounded, and the rest (`dayofweek`, `weekday`, `next_day`, the day shifts, comparisons) must decline nothing. With the bound disabled, all 5 tests fail. * **Coverage:** the 24 bounded coverage rows now name their domain, as `VarkaCoverageSuite` requires of any row whose kernel can decline. `coverage.json` and `docs/sql-varka.md` are regenerated, and the coverage differential's edges fixture straddles the bound. * **Range analysis:** `VarkaRangeAnalysisSuite` pins which columns are reported. * **Suites:** the gate's compile, wide, narrow, lint and quotes steps pass. At each vector width, 1,392 tests passed and none failed, with 34 cancelled as on every run, over 105 suites with the memory sanitizer on. Lint covers `dev/lint-java`, `dev/scalastyle`, Error Prone and NullAway. * **Option matrix:** the two configurations that change the guard's paths pass against those defaults: `exactRefusalStatus=true` (1,267 tests) and `guardUnderArm=false` (1,263 tests). Neither has a failure or a shape that stopped fusing. `VarkaEmittedBytesSuite`, `VarkaEmitCostSuite` and `VarkaEmitCostAuditSuite` pass. Only the 24 bounded coverage rows' bytes move; the emit cost tables and the audit are unchanged. * **Lint:** the pre-commit check and `dev/lint-java` are clean. * **Cost, how it got here.** The check went through three designs, each measured by a 10-round `VarkaThroughputBenchmark` pair against the base on GitHub runners (EPYC 7763, AVX2): * **A separate vectorised pass over each bounded column before the kernel.** [Run 38959217998](https://github.com/vecbricks/varka/actions/runs/38959217998) failed: `datediff + 1` was 17% slower by median and 11% by best, `year * 100 + month` 16% and 10%, `yearofweek` 14% and 10%, and `weekofyear` 15% and 9%. The pass read every bounded column from memory a second time. * **The same check moved into the kernel's loop, as two compares on the vector the kernel already loads.** Run 38994401183 passed the 10% threshold but left a residual on Varka's side only: `datediff + 1` 7% slower by median and 4% by best, `year * 100 + month` 7% and 4%, `yearofweek` 7% and 3%, `weekofyear` 6% and 7%. The Janino rows were flat by best. C2's assembly showed the cause. The benchmark's table has a null every 31st row, so every batch runs the masked loop, and there the guard spent nine instructions per lane group turning the validity bitmap into a mask, beside four for the compares. * **This PR's form: the stored days, by `min` and `max`.** Removing the validity read takes the masked `datediff + 1` loop from 630 to 368 instructions on AVX2, the same as its dense loop, and the dense guard drops from four ops a group to two. * **Laptop A/B, used only to choose the form; not a result.** These are timings of the kernel alone (`VarkaEmitDump --rounds`, 4,096-row batches, `-XX:UseAVX=2`, pinned to two cores, minimum of three runs on a shared machine, so a few percent is noise). Extra time over the kernel without any guard: | shape, body | compares with validity (previous) | compares without validity | min and max (this PR) | |---|---|---|---| | `year(d) * 100 + month(d)`, masked | +15% | +8% | +3% | | `year(d) * 100 + month(d)`, dense | +5% | +6% | +3% | | `weekofyear(d)`, masked | +8% | +3% | +3% | | `weekofyear(d)`, dense | +4% | +2% | +3% | On the cheapest kernel, `datediff + 1`, any per-lane check still costs about as much as the kernel itself. Every form folds into an accumulator that carries from one lane group to the next, so the check costs at least that accumulator's latency per group, and C2 doesn't split the chain when it unrolls. The re-pair below is what decides. * **Cost, the re-pair.** [Run 39038439214](https://github.com/vecbricks/varka/actions/runs/39038439214) paired this form against the base on an EPYC 7763: `VarkaThroughputBenchmark` and `VarkaEvaluatorOverheadBenchmark`, 10 rounds each. Both pass the 10% threshold with no row slower by both the median and the best. The four date rows of the earlier pairs, by median and by best: `datediff + 1` -0.5% and +1.9%, `year * 100 + month` -1.0% and -1.2%, `yearofweek` -1.8% and -1.1%, `weekofyear` -4.5% and -4.8%. So three of the four are within the plan's 3% by best (`VARKA-379.md` 11.4 #3) and `weekofyear` is not: about 4.6% slower in both measures, against 6% to 7% for the compare form and 14% to 15% for the separate pass. The Janino rows are flat, within 3% by median. The evaluator-overhead pair's 12 rows are all within 3% by median. The `row consumer` Varka rows show +28% to +31% by best with medians within 2%: one cold round on the base side, the cold-start spread of `m9/SCOPE.md` item 103, not this change. * **Review:** a `/code-review high` of the min-and-max change found two blockers before the gate did. The first version used the new form for every range guard over an int column, and the test grammar draws those with other bounds, so its random trees declined on poisoned null lanes. The compiled shapes' poisoned batches declined as well. Both are fixed: the form is now limited to the column bound, and the test harness stores the bound's own ends under nulls of a column read through it. The review also found a stale invariant comment, a doc that disagreed with the code and some style points, all fixed. Two points are left open: the generated reach reports don't list the new mechanism yet, and how warm-up treats a masked decline was not checked. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5)

MaxGekk added 2 commits April 14, 2018 16:12

Adding a test for ignoring cases of json fields

6aec10f

Making JSON reader respectful to the caseSensitive parameter

863ace7

MaxGekk closed this Sep 18, 2019

MaxGekk deleted the json-case-sensitive branch June 5, 2020 19:41

Navigation Menu

Repository navigation

Making JSON datasource respectful to spark.sql.caseSensitive - #3

Making JSON datasource respectful to spark.sql.caseSensitive#3
MaxGekk wants to merge 2 commits into
masterfrom
json-case-sensitive

MaxGekk commented Apr 14, 2018 •

edited

Loading

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MaxGekk commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MaxGekk commented Apr 14, 2018 •

edited

Loading