You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/09/10 18:49:55 UTC
[I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
alamb opened a new issue, #17508:
URL: https://github.com/apache/datafusion/issues/17508
the following PR adds uuids to certain substrait identifiers to disambiguate them, but this may make the plans non reproducable. @Blizzara has some ideas how how we can avoid the UUIDs
- https://github.com/apache/datafusion/pull/17299
FWIW, I looked a bit at what it'd take to fix the tracker. I think a core of the issue is that DF checks name ambiguity in two ways: there's the AmbiguousColumn exception you're running into, and then there is a `validate_unique_names()` function which gets called on the creation of the Project. The former needs unique non-qualified names, while the latter needs unique schema names (which _can_ be qualified).
An easy fix for the former would be to change `name_for_alias()` into `qualified_name()._1` here https://github.com/apache/datafusion/blob/1d9e13845021c2e82a012c2e83938c2a7661f295/datafusion/substrait/src/logical_plan/consumer/utils.rs#L398. However, that then regresses the latter check (including in the test case for this PR), since there will then be a project node with an expr `CAST(B.C as Utf8)` with a qualified name ([no qualifier], "B.C") and a schema name "B.C", as well as a reference to the original column `B.C` with a qualified name ("B", "C") and also schema name "B.C". As the qualified name's name parts are different, it wouldn't be renamed (after the change I propose), and then it'd fail the `validate_unique_names()` check. So maybe for a proper fix, NameTracker would need to track **both the schema name and the name-part of the qualified name**, and rename until both are unique.
(A simple example of the behavior of the CAST and validate_unique_names() is that `SELECT data.a, CAST(data.a as string) from data;` also fails in datafusion-cli.)
_Originally posted by @Blizzara in https://github.com/apache/datafusion/pull/17299#discussion_r2330926518_
--
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs closed issue #17508: Improve substrait NameTracker so it doesn't require uuids
URL: https://github.com/apache/datafusion/issues/17508
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "dd-annarose (via GitHub)" <gi...@apache.org>.
dd-annarose commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3613029097
take
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "dd-annarose (via GitHub)" <gi...@apache.org>.
dd-annarose commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3570511024
1. IIUC the name tracker should go over it and we'd have B.C and B.C__temp__0, or am I not understanding properly? Is is a hard requirement, even before the substrait parsing? I'm not sure then why we'd accept a query like `SELECT data.a, CAST(data.a as string) from data;`
2. I think your example is a bit of a mix between my two proposals; I was thinking of making CASTs appear in the case where we still don't want duplicate col names, so something like
```
SELECT B.C, CAST(B.C as TEXT) FROM table;
```
we'd get columns "B.C" and "B.C::TEXT"
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "dd-annarose (via GitHub)" <gi...@apache.org>.
dd-annarose commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3581188457
> Do you know why right now the name tracker is not triggering a rename if both schema names are the same? (which iiuc is what name_for_alias returns)
Right now, the name tracker does! I was unclear, sorry! I meant for the suggested solution from @alamb where we use `qualified_name().1`. Then, we'd need the cast to have the same qualified name as the column selection.
I'm not sure the issue that the alias fixes? aliasing with the qualified name? As far as I could tell, casts seem to be the only edge case to address.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3563938190
> making CAST(B.C as Utf8) have qualified name ("B", "C")
Keep in mind that two columns with the same qualified name and same field name, might result in an schema error as well (see [here](https://github.com/apache/datafusion/blob/e6ddb48f4017fdd7f94ff360695514934f4a5823/datafusion/common/src/dfschema.rs#L233)).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3564080258
> making CAST(B.C as Utf8) have qualified name ("B", "C")
IIRC two columns with the same qualified name and same field name, might result in an schema error as well during logical planning (see [here](https://github.com/apache/datafusion/blob/e6ddb48f4017fdd7f94ff360695514934f4a5823/datafusion/common/src/dfschema.rs#L233)).
> OR making it so that unaliased CASTs do appear in the field name in the schema, something like B.C::TEXT
if we had
```
SELECT
CAST(B.C as TEXT),
CAST(B.C as TEXT)
FROM table
```
could it still conflict?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "dd-annarose (via GitHub)" <gi...@apache.org>.
dd-annarose commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3559424424
do we want to make it possible for two columns in the output schema to have the same exact qualified name? Postgresql does allow this: `SELECT data.a, CAST(data.a as string) from data;` would yield 2 columns, both named `data.a`.
This is not a great feature, and I understand forbidding it. But this [PR](https://github.com/apache/datafusion/pull/3222) aligns DF on the behavior of Posgres & spark
> I would also like to match the behavior of Postgres and Spark where CAST does not appear in the field names in the schema
I think a first step towards a solution to this issue would either:
- making `CAST(B.C as Utf8)` have qualified name ("B", "C") so that the name tracker can detect column `B.C` has an already seen name
- OR making it so that unaliased CASTs do appear in the field name in the schema, something like `B.C::TEXT`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Improve substrait NameTracker so it doesn't require uuids [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on issue #17508:
URL: https://github.com/apache/datafusion/issues/17508#issuecomment-3575072353
Sorry I think I misunderstood the issue, I graphed this to better picture this, please let me know if I'm understanding this wrong
`SELECT B.C, CAST(B.C as TEXT) FROM table;
` yields to the following structure
```
╔═══════════════════╦══════════════╦══════════════╦═══════════════╗
║ Expression ║ Qualifier ║ qualified_ ║ schema_name() ║
║ ║ .0 ║ name().1 ║ ║
╠═══════════════════╬══════════════╬══════════════╬═══════════════╣
║ Column(B.C) ║ Some("B") ║ "C" ║ "B.C" ║
║ ║ ↑ ║ ↑ ║ ↑ ║
║ ║ Has table! ║ Field only ║ Combined ║
╠═══════════════════╬══════════════╬══════════════╬═══════════════╣
║ CAST(B.C as Utf8) ║ None ║ "B.C" ║ "B.C" ║
║ ║ ↑ ║ ↑ ║ ↑ ║
║ ║ No table! ║ Full string ║ Same string ║
╚═══════════════════╩══════════════╩══════════════╩═══════════════╝
```
The null literal situation from the issue description yields to the following structure (without the uuid workaround):
```
╔═══════════════════════════╦═══════════════╦═══════════════╦═══════════════════╗
║ Expression ║ Qualifier ║ qualified_ ║ schema_name() ║
║ ║ .0 ║ name().1 ║ (name_for_alias) ║
╠═══════════════════════════╬═══════════════╬═══════════════╬═══════════════════╣
║ lit(NULL) ║ None ║ "UTF8(NULL)" ║ "UTF8(NULL)" ║
║ (new literal in project) ║ ║ ║ ║
╠═══════════════════════════╬═══════════════╬═══════════════╬═══════════════════╣
║ Column(left.UTF8(NULL)) ║ Some("left") ║ "UTF8(NULL)" ║ "left.UTF8(NULL)" ║
║ (from input after join) ║ ║ ║ ║
╚═══════════════════════════╩═══════════════╩═══════════════╩═══════════════════╝
```
The solutions proposed in the issue description -> using qualified_name().1 instead of name_for_alias() to detect when to rename would directly fix the null literal case wihtout needing to use uuids. But for the cast situation the name tracker would never trigger a rename since it sees a different qualified_name().1, and because the schema_name are the same they fail later in `validate_unique_names`
iiuc we want to solve both situations while avoiding the uuid workaround.
> do we want to make it possible for two columns in the output schema to have the same exact qualified name?
Unless I'm missing something, I don't see the harm in allowing it if Postgres already does it.
> making CAST(B.C as Utf8) have qualified name ("B", "C") so that the name tracker can detect column B.C has an already seen name
Do you know why right now the name tracker is not triggering a rename if both schema names are the same? (which iiuc is what `name_for_alias` returns)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org