Skip to content

revert [SPARK-43752] and add test#55348

Open
cloud-fan wants to merge 2 commits intoapache:masterfrom
cloud-fan:SPARK-43752-revert
Open

revert [SPARK-43752] and add test#55348
cloud-fan wants to merge 2 commits intoapache:masterfrom
cloud-fan:SPARK-43752-revert

Conversation

@cloud-fan
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan commented Apr 15, 2026

What changes were proposed in this pull request?

This PR reverts the code changes from #55127 (commit eca57ea) and moves the test to InsertIntoTests where it properly runs across V2 INSERT SQL test suites.

Specifically:

  • Removes the V2WriteCommand case added to Analyzer.ResolveReferences and ResolveColumnDefaultInCommandInputQuery
  • Removes the test from ResolveDefaultColumnsSuite (wrong location for a V2 end-to-end test)
  • Adds the test to InsertIntoSQLOnlyTests in InsertIntoTests.scala, which runs in suites with includeSQLOnlyTests = true: DataSourceV2SQLSuite, DataSourceV2SQLSessionCatalogSuite, and V1WriteFallbackSessionCatalogSuite. This is correct since DEFAULT is SQL-only syntax.

Why are the changes needed?

The code added by #55127 is dead code. For INSERT INTO v2_table VALUES (1, DEFAULT), the resolution flow is:

  1. The parser produces InsertIntoStatement for both V1 and V2 tables.
  2. In the Resolution batch, ResolveReferences dispatches InsertIntoStatement to resolveColumnDefaultInCommandInputQuery, which resolves DEFAULT using the table schema. For V2 tables, the DataSourceV2Relation schema includes column default metadata (via CatalogV2Util.encodeDefaultValue), so DEFAULT is resolved correctly at this stage.
  3. ResolveInsertInto only converts InsertIntoStatement to a V2WriteCommand (AppendData/OverwriteByExpression/OverwritePartitionsDynamic) after the query is fully resolved — by which point DEFAULT is already gone.

No code path (SQL or DataFrame API) creates a V2WriteCommand with unresolved DEFAULT attributes. The PlanResolutionSuite test "INSERT INTO table with default column value" already verifies this resolution at the plan level.

The test is still valuable and passes without the code changes — this PR moves it to the proper location.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Moved the existing test to InsertIntoSQLOnlyTests in InsertIntoTests.scala. The test verifies:

  • INSERT INTO with partial DEFAULT values
  • INSERT INTO with all DEFAULT values
  • INSERT OVERWRITE with DEFAULT values

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code 4.6

…mmands

The code added by SPARK-43752 to resolve column DEFAULT in V2WriteCommand (AppendData, OverwriteByExpression, OverwritePartitionsDynamic) is dead code. For `INSERT INTO v2_table VALUES (1, DEFAULT)`, the resolution flow is:

1. The parser produces `InsertIntoStatement` (for both V1 and V2 tables).
2. `ResolveReferences` resolves DEFAULT via `resolveColumnDefaultInCommandInputQuery` while still in the `InsertIntoStatement` stage.
3. `ResolveInsertInto` only converts to `V2WriteCommand` after the query is fully resolved — by which point DEFAULT is already gone.

No code path creates a V2WriteCommand with unresolved DEFAULT attributes.

This reverts the code changes from eca57ea and moves the test to InsertIntoTests where it runs across all V2 INSERT test suites.

Co-authored-by: Isaac
@cloud-fan cloud-fan changed the title [SPARK-43752][SQL] Remove dead code for column DEFAULT in V2 write commands revert [SPARK-43752] and add test Apr 15, 2026
@cloud-fan
Copy link
Copy Markdown
Contributor Author

cc @LuciferYang @dongjoon-hyun

}
}

test("SPARK-43752: InsertInto: column DEFAULT values") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to retain the "SPARK-43752:" prefix?

@LuciferYang
Copy link
Copy Markdown
Contributor

Removes the stale TODO (SPARK-43752) comment (since the V2 path is already covered)

This TODO already remove in #55127

@LuciferYang
Copy link
Copy Markdown
Contributor

LuciferYang commented Apr 15, 2026

Adds the test to InsertIntoSQLOnlyTests in InsertIntoTests.scala, which runs across DataSourceV2SQLSuite, DataSourceV2DataFrameSuite, session catalog suites, and V1 fallback suites

The new test is inside the if (includeSQLOnlyTests) guard, so it only runs in suites with includeSQLOnlyTests = true:

  • DataSourceV2SQLSuite — yes
  • DataSourceV2SQLSessionCatalogSuite — yes
  • V1WriteFallbackSessionCatalogSuite — yes
  • DataSourceV2DataFrameSuite (includeSQLOnlyTests = false) — no
  • DataSourceV2DataFrameSessionCatalogSuite (includeSQLOnlyTests = false) — no

This is correct behavior since DEFAULT is SQL-only syntax, but the description overstates the coverage. Might be worth updating it to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants