Reapply "Fix/support duplicate column names"

[alamb]

Draft until I have updated the datafusion-testing repo

Which issue does this PR close?

• Closes Support columns having the same alias #6543 (again)

Rationale for this change

• @RafaelHerrero introduced this new feature in Fix/support duplicate column names #6543 #21126
• @mbutrovich had to revert it due to extended test failures here here fix: Revert "Fix/support duplicate column names #6543 (#21126)" #21254

I think it is an important feature so let's get it in

What changes are included in this PR?

1. Reapply the change (revert the revert)
2. Update the extended tests expected output as described in the readme

Are these changes tested?

Yes by CI (and @mbutrovich has updated the tests to run extended tests here #21255)

Are there any user-facing changes?

[alamb]

I tried to update the sqllogictests following the instructions

However, it seems as if our patched version of sqlogictest doesn't match the version that we currently use:

 [[package]]
 name = "sqllogictest"
-version = "0.29.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d03b2262a244037b0b510edbd25a8e6c9fb8d73ee0237fc6cc95a54c16f94a82"
+version = "0.27.2"
+source = "git+https://github.com/Omega359/sqllogictest-rs.git?rev=73c47cf7#73c47cf74944064173f6b603cb39c3802b46b0c7"
 dependencies = [
  "async-trait",
  "educe",

I am going to turn codex loose on this for a bit and see what it comes up with

[RafaelHerrero]

Since this was my original change, I'd like to help get it re-landed. I can look into the sqllogictest version mismatch and update the expected results if that would be helpful.

[alamb]

Since this was my original change, I'd like to help get it re-landed. I can look into the sqllogictest version mismatch and update the expected results if that would be helpful.

That would be super helpful -- I haven't had a chance to look into it more yet

[RafaelHerrero]

Hi @alamb, I dug into the sqllogictest version mismatch and found a way to update the extended tests without needing the Omega359 fork.

The problem with the fork approach:
The regenerate_sqlite_files.sh script swaps in a forked sqllogictest at v0.27.2, but main now uses v0.29.1. The APIs changed, so the fork doesn't compile.

What I found:
The standard --complete mode (v0.29.1) works and generates correct results, but it has two issues with the SQLite test files:

1. It doesn't respect control resultmode valuewise — writes space-separated rows instead of one value per line
2. Hash values get computed with the wrong sort order for newly-generated blocks

The fix (2-pass approach):

1. Run --complete on all SQLite files to generate results
2. Post-process with a script that only touches blocks that were query error ... Projections require unique expression names — converts results to valuewise format with valuesort, leaving all existing blocks untouched
3. Run --complete again so hash values are recomputed with the correct sort mode
4. Post-process again to re-apply valuewise format

Results:

• All 595 SQLite test files pass locally
• 279 files in datafusion-testing need updating (~39,706 query error blocks that now succeed or have a different error)
• No changes needed to datafusion beyond the code fix (revert of revert)

Full disclosure: I used Claude Code to help debug this and write the post-processing script — it took quite a bit of iteration to figure out the valuewise format and hash issues.

Proposed next steps:

1. Open a PR to apache/datafusion-testing with the 279 updated .slt files
2. Once merged, update the submodule reference in this PR

Let me know if this approach looks good and I'll open the datafusion-testing PR.

[alamb]

Thank you so much @RafaelHerrero -- this sounds right to me

Proposed next steps:

1. Open a PR to apache/datafusion-testing with the 279 updated .slt files
2. Once merged, update the submodule reference in this PR

Yes, thank you -- this sounds right

Can I also request a step 3 (which is to update the datafusion/sqllogictest/regenerate_sqlite_files.sh so it uses an updated fork of sqllogictests)? I can make a fork for us in datafusion-contrib perhaps

I also wonder if there is something we can upstream to the sqllogictest repo so we don't have to maintain a fork

[RafaelHerrero]

Good news on step 3 — the Omega359 fork features were already upstreamed to risinglightdb/sqllogictest-rs in v0.24.0 (PR #237), and DataFusion is already on v0.29.1. So no new fork needed, we just need to remove the fork swap from regenerate_sqlite_files.sh (line 167).

The regenerate/sqllogictests.rs also needs updating (the non-postgres complete path is commented out), but that can be a separate follow-up.

I'll get the datafusion-testing PR up next. Want me to include the script cleanup in the datafusion PR or keep it separate?

[alamb]

Good news on step 3 — the Omega359 fork features were already upstreamed to risinglightdb/sqllogictest-rs in v0.24.0 (PR #237), and DataFusion is already on v0.29.1. So no new fork needed, we just need to remove the fork swap from regenerate_sqlite_files.sh (line 167).

Awesome!

The regenerate/sqllogictests.rs also needs updating (the non-postgres complete path is commented out), but that can be a separate follow-up.

I'll get the datafusion-testing PR up next. Want me to include the script cleanup in the datafusion PR or keep it separate?

If possible, can you make PRs:

1. A PR to update the scritps
2. A PR to update the expected tests in datafusion-testing

Then we can update this PR with an updated pin of datafusion-testing

[RafaelHerrero]

Hey @alamb, both PRs are up:

1. datafusion-testing: Update expected results for duplicate column names fix datafusion-testing#19 — updated expected results (279 files)
2. Script cleanup: Remove sqllogictest fork swap from regenerate_sqlite_files.sh #21578 — removes the fork swap from regenerate_sqlite_files.sh

Let me know if anything needs adjusting.

[alamb]

Script cleanup: #21578 — removes the fork swap from regenerate_sqlite_files.sh

Sounds good -- I gave this a review and left some comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.