Uh oh!
There was an error while loading. Please reload this page.
[SPARK-56619][SQL][TESTS] Add DSv2 repeated table access tests with internal/external changes in Classic/Connect mode - #55462
Closed
longvu-db wants to merge 36 commits into
Closed
Conversation
longvu-db
marked this pull request as draft
April 21, 2026 20:48
longvu-dbforce-pushed
the
dsv2-pr2-repeated-sql
branch
from
April 24, 2026 14:42
77c6144 to
242bdaaComparelongvu-db
marked this pull request as ready for review
April 24, 2026 15:15
longvu-dbforce-pushed
the
dsv2-pr2-repeated-sql
branch
from
April 30, 2026 16:52
4b5c3a5 to
489e519Compareandreaschat-db
approved these changes
May 12, 2026
andreaschat-db
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Please update the PR description with the correct number of tests added.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
longvu-dbforce-pushed
the
dsv2-pr2-repeated-sql
branch
3 times, most recently
from
May 26, 2026 07:54
5725ba5 to
b06c979Comparecloud-fan
approved these changes
May 26, 2026
cloud-fan
left a comment
Contributor
There was a problem hiding this comment.
Test-only PR. Adds 9 new tests (18 total across classic + Connect) verifying that repeated session.sql() calls reflect external mutations made via the DSv2 catalog API. Follows the trait-extraction pattern established by DSv2TempViewWithStoredPlanTests / DSv2ExternalMutationTestBase. Scenarios cover external writes, external ADD COLUMN, and external drop/recreate, each with a session-mutation baseline and a caching-connector stale-read variant.
Caching-correctness mechanics line up: copyOnLoad=true + CachingInMemoryTableCatalog.loadTable caches the first snapshot, while the write-priv loadTable(ident, INSERT) variant used by externalAppend bypasses the cache and mutates the live table — so the cached snapshot stays stale until REFRESH TABLE invokes invalidateTable. Connect-side cleanup correctly relies on per-test session isolation (SparkConnectServerTest.afterEach invalidates all sessions).
A few nit suggestions inline. LGTM.
(Prior approval/inline nits on commit dc9cca7 predate the trait extraction in 40a0966; current head is d9ac5df and those line numbers no longer exist.)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 26, 2026
…ming/Scaladoc - Delete separate DataSourceV2CacheTableConnectSuite; mix DSv2CacheTableTests into existing DataSourceV2TempViewConnectSuite (same pattern as classic DataSourceV2DataFrameSuite mixing in multiple traits). - Rename private val T -> testTable for consistency with PR apache#55462. - Fix Scaladoc: list all 5 scenarios, say "session and external mutations". - Fix NOTE: "DDL / DML" not just "DDL". Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 26, 2026
…tBase Same change as PR apache#55462: shared namespace fixture in the base trait so consumer traits don't duplicate them. Co-authored-by: Isaac
cloud-fan
left a comment
Contributor
There was a problem hiding this comment.
2 addressed, 0 remaining, 1 new (newly introduced from the hoist).
Addressed since the prior review: the lead-sentence framing and the "DDL / DML" wording in DSv2RepeatedTableAccessTests.scala. The hoist of T / CT / testIdent to DSv2ExternalMutationTestBase is the right move.
One new issue: the find-replace that did the hoist updated $T / $CT inside string interpolations but missed the bare references in session.table(T) / session.table(CT) calls. DSv2TempViewWithStoredPlanTests.scala no longer compiles. CI hasn't run on this commit, so it wasn't caught.
Uh oh!
There was an error while loading. Please reload this page.
longvu-dbforce-pushed
the
dsv2-pr2-repeated-sql
branch
from
May 26, 2026 13:56
92c571f to
6aab5ccComparelongvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 26, 2026
…ming/Scaladoc - Delete separate DataSourceV2CacheTableConnectSuite; mix DSv2CacheTableTests into existing DataSourceV2TempViewConnectSuite (same pattern as classic DataSourceV2DataFrameSuite mixing in multiple traits). - Rename private val T -> testTable for consistency with PR apache#55462. - Fix Scaladoc: list all 5 scenarios, say "session and external mutations". - Fix NOTE: "DDL / DML" not just "DDL". Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 26, 2026
…tBase Same change as PR apache#55462: shared namespace fixture in the base trait so consumer traits don't duplicate them. Co-authored-by: Isaac
Adds 6 tests to verify that repeated sql() calls in Spark Connect mode always see the latest table state. This mirrors the classic-mode tests added to DataSourceV2DataFrameSuite, covering: 1. Session write: INSERT via SQL, next SELECT sees new row 2. External write: data added via catalog API, next SELECT sees it 3. Session schema change: ADD COLUMN via SQL, next SELECT sees new schema 4. External schema change: column added via catalog API, next SELECT sees it 5. Session drop/recreate: DROP+CREATE via SQL, next SELECT sees empty table 6. External drop/recreate: drop+create via catalog API, next SELECT sees empty In Connect, every sql() call creates a fresh plan re-analyzed on the server, so all modifications are always visible. Co-authored-by: Isaac
Co-authored-by: Isaac
Add 3 new tests that create a DataFrame ONCE and reuse it across external mutations (data write, schema change, drop/recreate). These tests would FAIL in classic Spark (where the resolved plan is captured at DataFrame creation time) but PASS in Connect (where each action re-sends the plan for fresh server-side analysis). This makes the suite genuinely Connect-specific, not just a copy of the classic tests with Connect infrastructure. Co-authored-by: Isaac
Co-authored-by: Isaac
Adds 3 Connect tests verifying that when a DSv2 connector caches table state, external changes are invisible through repeated sql() calls. Co-authored-by: Isaac
Co-authored-by: Isaac
CachingInMemoryTableCatalog.clearCache() must run AFTER DROP TABLE, not before. Otherwise DROP TABLE re-caches the table via loadTable(), and the next test's CREATE TABLE hits a stale cache entry. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
…panion object CachingInMemoryTableCatalog has no companion object, so the static call CachingInMemoryTableCatalog.clearCache() does not compile. Fixed by getting the server-side catalog instance and calling clearCache() on it, matching the pattern used by the classic DataSourceV2DataFrameSuite. Co-authored-by: Isaac
Co-authored-by: Isaac
The withCleanup method was unconditionally accessing the cachingcat catalog to clear its cache, but the Connect server session only loads a catalog when first accessed via SQL. Tests using testcat never trigger loading cachingcat, causing CatalogNotFoundException. Co-authored-by: Isaac
…ect suites Move the 9 shared repeated-sql() test cases into a new DSv2RepeatedTableAccessTests trait (extending DSv2ExternalMutationTestBase), matching the pattern established by DSv2TempViewWithStoredPlanTests. Classic mode: mixed into DataSourceV2DataFrameSuite (testPrefix = ""). Connect mode: DataSourceV2RepeatedSQLConnectSuite now extends the trait and only keeps the 3 Connect-specific DataFrame-reuse tests inline. Co-authored-by: Isaac
…estIdent - Update DSv2ExternalMutationTestBase Scaladoc to reference both DSv2TempViewWithStoredPlanTests and DSv2RepeatedTableAccessTests. - Rename private val ident to testIdent in Connect suite for consistency with the shared trait's naming. Co-authored-by: Isaac
…kAnswer Co-authored-by: Isaac
The shared trait's repeated sql() tests already exercise Connect's re-analysis behavior. The DF reuse tests were testing the same code path since Connect re-sends the plan on every action regardless. Co-authored-by: Isaac
…o existing suite Instead of a separate DataSourceV2RepeatedSQLConnectSuite with identical boilerplate, add 'with DSv2RepeatedTableAccessTests' to the existing DataSourceV2TempViewConnectSuite. This mirrors how the classic DataSourceV2DataFrameSuite mixes in both traits. Co-authored-by: Isaac
…ctSuite The suite now runs both temp view and repeated table access tests, so the old name was misleading. The new name mirrors the classic DataSourceV2DataFrameSuite. Co-authored-by: Isaac
Co-authored-by: Isaac
The tests are about repeated table access with external changes, not generic session writes. Updated Scaladoc to describe all three external mutation scenarios and the caching-connector variant. Co-authored-by: Isaac
…ternal mutation Co-authored-by: Isaac
…L note - Hoist T, CT, testIdent to DSv2ExternalMutationTestBase as protected vals (testTable, cachingTestTable, testIdent) so both consumer traits share the same namespace fixture. - Fix Scaladoc lead sentence: "session and external mutations" (not just external). - Fix NOTE: "DDL / DML" (not just DDL, since .collect() is also appended to INSERT and REFRESH TABLE). Co-authored-by: Isaac
The hoist of constants to DSv2ExternalMutationTestBase updated string interpolations ($T/$CT) but missed bare references in session.table(T) and session.table(CT) calls. Co-authored-by: Isaac
Co-authored-by: Isaac
longvu-dbforce-pushed
the
dsv2-pr2-repeated-sql
branch
from
May 26, 2026 15:12
6aab5cc to
a9cd1baCompareCo-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 26, 2026
…ly constructed queries Rebase onto latest master. Restructure into trait pattern (DSv2JoinRefreshTests) matching the approach from PR apache#55462 (DSv2TempViewWithStoredPlanTests). Tests that work in both classic and Connect modes live in the shared trait. Tests that rely on classic-mode eager analysis live in the classic suite directly. Co-authored-by: Isaac
cloud-fan
commented
May 27, 2026
Contributor
thanks, merging to master/4.x/4.2 (test only) |
cloud-fan pushed a commit
that referenced
this pull request
May 27, 2026
…ternal/external changes in Classic/Connect mode ### What changes were proposed in this pull request? Add 9 tests verifying that repeated `sql()` calls on DSv2 tables correctly reflect external changes made via the catalog API, covering both classic and Connect modes (so 18 tests in total) `DSv2RepeatedTableAccessTests` extends `DSv2ExternalMutationTestBase`, following the same pattern as `DSv2TempViewWithStoredPlanTests` (PR #55571). The tests cover three external mutation scenarios, each with a session mutation baseline, an external mutation test, and a caching-connector variant: - **Scenario 1 (external writes)**: External data appended via the catalog API is visible to subsequent `sql()` queries. - **Scenario 2 (external schema changes)**: External ADD COLUMN via the catalog API is visible to subsequent `sql()` queries. - **Scenario 3 (external drop/recreate)**: External drop and recreate via the catalog API resolves to the new empty table. For each scenario, the caching-connector variant (`cachingcat`) demonstrates that a connector with its own `loadTable` cache returns stale results until `REFRESH TABLE` invalidates the cache. #### New files - **`DSv2RepeatedTableAccessTests`**: Shared trait containing all 9 tests (3 scenarios x 3 variants: session mutation baseline, external mutation, caching connector), using `session.sql(...)` with `.collect()` calls (harmless in classic mode, required for Connect). #### Modified files - **`DataSourceV2DataFrameSuite`**: Mixes in `DSv2RepeatedTableAccessTests` (classic runner, `testPrefix = ""`). - **`DataSourceV2TempViewConnectSuite` -> `DataSourceV2DataFrameConnectSuite`**: Renamed and now mixes in both `DSv2TempViewWithStoredPlanTests` and `DSv2RepeatedTableAccessTests`. This is the Connect counterpart of the classic `DataSourceV2DataFrameSuite`. - **`DSv2ExternalMutationTestBase`**: Scaladoc updated to reference both consumer traits. ### Why are the changes needed? These tests document and lock down the expected behavior: repeated `sql()` access without CACHE TABLE always sees the latest table state after external mutations. This prevents regressions if internal resolution or caching logic changes. The trait extraction enables Connect-mode reuse without duplicating test logic. ### Does this PR introduce _any_ user-facing change? No. This PR is test-only. ### How was this patch tested? 9 new tests run in both classic and Connect modes: Classic: ``` build/sbt 'sql/testOnly *DataSourceV2DataFrameSuite -- -z "repeated sql()"' ``` Connect: ``` build/sbt 'connect/server/testOnly *DataSourceV2DataFrameConnectSuite -- -z "repeated sql()"' ``` ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (claude-opus-4-6) Closes#55462 from longvu-db/dsv2-pr2-repeated-sql. Authored-by: Thang Long Vu <long.vu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e40d175) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan pushed a commit
that referenced
this pull request
May 27, 2026
…ternal/external changes in Classic/Connect mode ### What changes were proposed in this pull request? Add 9 tests verifying that repeated `sql()` calls on DSv2 tables correctly reflect external changes made via the catalog API, covering both classic and Connect modes (so 18 tests in total) `DSv2RepeatedTableAccessTests` extends `DSv2ExternalMutationTestBase`, following the same pattern as `DSv2TempViewWithStoredPlanTests` (PR #55571). The tests cover three external mutation scenarios, each with a session mutation baseline, an external mutation test, and a caching-connector variant: - **Scenario 1 (external writes)**: External data appended via the catalog API is visible to subsequent `sql()` queries. - **Scenario 2 (external schema changes)**: External ADD COLUMN via the catalog API is visible to subsequent `sql()` queries. - **Scenario 3 (external drop/recreate)**: External drop and recreate via the catalog API resolves to the new empty table. For each scenario, the caching-connector variant (`cachingcat`) demonstrates that a connector with its own `loadTable` cache returns stale results until `REFRESH TABLE` invalidates the cache. #### New files - **`DSv2RepeatedTableAccessTests`**: Shared trait containing all 9 tests (3 scenarios x 3 variants: session mutation baseline, external mutation, caching connector), using `session.sql(...)` with `.collect()` calls (harmless in classic mode, required for Connect). #### Modified files - **`DataSourceV2DataFrameSuite`**: Mixes in `DSv2RepeatedTableAccessTests` (classic runner, `testPrefix = ""`). - **`DataSourceV2TempViewConnectSuite` -> `DataSourceV2DataFrameConnectSuite`**: Renamed and now mixes in both `DSv2TempViewWithStoredPlanTests` and `DSv2RepeatedTableAccessTests`. This is the Connect counterpart of the classic `DataSourceV2DataFrameSuite`. - **`DSv2ExternalMutationTestBase`**: Scaladoc updated to reference both consumer traits. ### Why are the changes needed? These tests document and lock down the expected behavior: repeated `sql()` access without CACHE TABLE always sees the latest table state after external mutations. This prevents regressions if internal resolution or caching logic changes. The trait extraction enables Connect-mode reuse without duplicating test logic. ### Does this PR introduce _any_ user-facing change? No. This PR is test-only. ### How was this patch tested? 9 new tests run in both classic and Connect modes: Classic: ``` build/sbt 'sql/testOnly *DataSourceV2DataFrameSuite -- -z "repeated sql()"' ``` Connect: ``` build/sbt 'connect/server/testOnly *DataSourceV2DataFrameConnectSuite -- -z "repeated sql()"' ``` ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (claude-opus-4-6) Closes#55462 from longvu-db/dsv2-pr2-repeated-sql. Authored-by: Thang Long Vu <long.vu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e40d175) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 27, 2026
…ly constructed queries Rebase onto latest master. Restructure into trait pattern (DSv2JoinRefreshTests) matching the approach from PR apache#55462 (DSv2TempViewWithStoredPlanTests). Tests that work in both classic and Connect modes live in the shared trait. Tests that rely on classic-mode eager analysis live in the classic suite directly. Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 27, 2026
…ly constructed queries Rebase onto latest master. Restructure into trait pattern (DSv2JoinRefreshTests) matching the approach from PR apache#55462 (DSv2TempViewWithStoredPlanTests). Tests that work in both classic and Connect modes live in the shared trait. Tests that rely on classic-mode eager analysis live in the classic suite directly. Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 28, 2026
…ly constructed queries Rebase onto latest master. Restructure into trait pattern (DSv2JoinRefreshTests) matching the approach from PR apache#55462 (DSv2TempViewWithStoredPlanTests). Tests that work in both classic and Connect modes live in the shared trait. Tests that rely on classic-mode eager analysis live in the classic suite directly. Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/spark
that referenced
this pull request
May 28, 2026
…ly constructed queries Rebase onto latest master. Restructure into trait pattern (DSv2JoinRefreshTests) matching the approach from PR apache#55462 (DSv2TempViewWithStoredPlanTests). Tests that work in both classic and Connect modes live in the shared trait. Tests that rely on classic-mode eager analysis live in the classic suite directly. Co-authored-by: Isaac
longvu-db added a commit
to longvu-db/delta
that referenced
this pull request
Jun 5, 2026
…e-REFRESH state, trim error helper - Rename repeated-access tests to match apache/spark#55462 (keep scenario numbers) - Assert the table state before REFRESH TABLE in the external scenarios - Simplify checkError mismatch message to a single line
murali-db pushed a commit
to delta-io/delta
that referenced
this pull request
Jun 9, 2026
…ernal changes in Classic/Connect mode (#6921) ## Description Add 6 tests verifying that repeated `sql()` calls on Delta tables correctly reflect external changes made via the catalog API, covering both classic and Connect modes (so 24 tests in total: 12 classic + 12 Connect). Related Spark [PR](apache/spark#55462), tested with an mock Connector. ### Shared external-write simulation Following Spark's `DSv2ExternalMutationTestBase`, the external-write simulation functions in `DeltaTableRefreshSharedBase` write commit files directly into the table's `_delta_log` on the local filesystem (shared by the Connect client and server) using only `java.io` plus `DataFrame.write`. This bypasses the in-process DeltaLog/snapshot cache in both modes, so a fresh `sql()` re-resolves and observes the change. ### Scenarios Session writes and external writes, each for: (1) data append, (2) schema change via add-column, (3) drop + recreate. Every scenario verifies repeated `sql()` access reflects the latest snapshot. Concrete suites are parameterized by `V2_ENABLE_MODE` (AUTO, STRICT) in both classic and Connect. ### STRICT behavior Under STRICT the V2 Kernel connector behaves the same as AUTO, with one exception: `ADD COLUMN` is not supported in V2 yet. After an in-session `ALTER TABLE ADD COLUMN`, the follow-up `INSERT` resolves against the stale cached schema and fails with `INSERT_COLUMN_ARITY_MISMATCH`. This is asserted with a `TODO` to flip once V2 refreshes its schema. ## How was this patch tested? Tests only. `spark/Test/compile` and `connectClient/Test/compile` pass, and all suites pass locally (12 classic + 12 Connect). They also run in CI in both modes. ## Does this PR introduce _any_ user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add 9 tests verifying that repeated
sql()calls on DSv2 tables correctly reflect external changes made via the catalog API, covering both classic and Connect modes (so 18 tests in total)DSv2RepeatedTableAccessTestsextendsDSv2ExternalMutationTestBase, following the same pattern asDSv2TempViewWithStoredPlanTests(PR #55571). The tests cover three external mutation scenarios, each with a session mutation baseline, an external mutation test, and a caching-connector variant:sql()queries.sql()queries.For each scenario, the caching-connector variant (
cachingcat) demonstrates that a connector with its ownloadTablecache returns stale results untilREFRESH TABLEinvalidates the cache.New files
DSv2RepeatedTableAccessTests: Shared trait containing all 9 tests (3 scenarios x 3 variants: session mutation baseline, external mutation, caching connector), usingsession.sql(...)with.collect()calls (harmless in classic mode, required for Connect).Modified files
DataSourceV2DataFrameSuite: Mixes inDSv2RepeatedTableAccessTests(classic runner,testPrefix = "").DataSourceV2TempViewConnectSuite->DataSourceV2DataFrameConnectSuite: Renamed and now mixes in bothDSv2TempViewWithStoredPlanTestsandDSv2RepeatedTableAccessTests. This is the Connect counterpart of the classicDataSourceV2DataFrameSuite.DSv2ExternalMutationTestBase: Scaladoc updated to reference both consumer traits.Why are the changes needed?
These tests document and lock down the expected behavior: repeated
sql()access without CACHE TABLE always sees the latest table state after external mutations. This prevents regressions if internal resolution or caching logic changes. The trait extraction enables Connect-mode reuse without duplicating test logic.Does this PR introduce any user-facing change?
No. This PR is test-only.
How was this patch tested?
9 new tests run in both classic and Connect modes:
Classic:
Connect:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-6)