-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix ambiguous column names in substrait conversion as a result of literals having the same name during conversion. #17299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
d09586b
4e6688d
39b159a
517425d
5581ddf
52f6832
619cced
d39522c
32f10b0
0d13ab2
40f2602
aacbe31
a6143f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,7 +62,17 @@ pub async fn from_project_rel( | |||||||||||||
| // to transform it into a column reference | ||||||||||||||
| window_exprs.insert(e.clone()); | ||||||||||||||
| } | ||||||||||||||
| explicit_exprs.push(name_tracker.get_uniquely_named_expr(e)?); | ||||||||||||||
| // Since substrait removes aliases, we need to assign literals with a UUID alias to avoid | ||||||||||||||
| // ambiguous names when the same literal is used before and after a join. | ||||||||||||||
| // The name tracker will ensure that two literals in the same project would have | ||||||||||||||
| // unique names but, it does not ensure that if a literal column exists in a previous | ||||||||||||||
| // project say before a join that it is deduplicated with respect to those columns. | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify my understanding of this. The name tracker guarantees name uniqueness in the output names of the relation it is applied to (Projects, Aggregates). In cases of relations that consume multiple inputs (Joins, Unions), if the individual inputs have names that are unique but duplicated between them, we get duplicate name issues in the output schema when we combine the input schemas?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes so the name tracker makes sure that if you have a project that creates two null string columns, it will create two unique names for those two columns. But, say you create one of those null columns before a join and then another in a project immediately after a join, the plan fails with an ambiguous column error because there is a UTF8(NULL) from say the left and then another UTF8(NULL) from the project after the join which has no source and it's therefore an ambiguous reference.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're also correct that if you have a null in the left and the right side of a join then this will also be an issue today
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Interesting, I would have expected datafusion/datafusion/substrait/src/logical_plan/consumer/rel/project_rel.rs Lines 74 to 78 in 3d5863b
to generate a unique name in that scenario.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the problem is the NameTracker doesn't ignore qualifiers, but the "ambiguous schema" check does. Thus if the input to the Project has e.g. "table1.NULL" column and adds a "NULL" column (from I think my recommendation would be to make the NameTracker more robust instead, so that it ignores the qualifiers at least when there is also a non-qualified name. While this UUID-aliasing of literals seems like it should work for this specific case, I can imagine there might be some other case where the clash happens with non-literal columns (though I'm not able to come up with an example right now). (Also hey 👋 @xanderbailey!)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated some of the comments made by @vbarua. I'm happy to merge as is to unblock us and then I can have a go at improving the name tracker in a follow-up?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 1 is perfectly reasonable. This is still an improvement even if it doesn't solve every case, and we can always iterate it on it further.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine by me. 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 An easy fix for the former would be to change
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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alamb seems like we're okay to merge as it is.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I filed a follow on ticket to track the improvement idea here: |
||||||||||||||
| // See: https://github.com/apache/datafusion/pull/17299 | ||||||||||||||
| let maybe_apply_alias = match e { | ||||||||||||||
| lit @ Expr::Literal(_, _) => lit.alias(uuid::Uuid::new_v4().to_string()), | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one thing that's a bit wonky about this is that the usage of UUIDs injects a little bit of randomness into the conversion from Substrait plan to Datafusion plan. You're already dealing with this in your tests by using your filter for eliding UUID values: let mut settings = insta::Settings::clone_current();
settings.add_filter(
r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}",
"[UUID]",
);Using UUIDs makes it easy to guarantee that names are unique, but make the plans less readable and can complicate testing. Figuring out a deterministic scheme for this would be nice. We could potentially apply the name tracker to the inputs of multi-input relations (i.e. JoinRel, CrossRel, SetRel). That's not as nice as the UUID solution you have because it would require extra handling in every multi-input relation, but it could potentially improve readability. I'm not wedded to this though. The UUID solutions works well enough and maybe in practice it won't be that much of an issue. If it does, we can always tweak the plan conversion later. |
||||||||||||||
| _ => e, | ||||||||||||||
| }; | ||||||||||||||
| explicit_exprs.push(name_tracker.get_uniquely_named_expr(maybe_apply_alias)?); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| let input = if !window_exprs.is_empty() { | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,6 +144,47 @@ mod tests { | |
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn null_literal_before_and_after_joins() -> Result<()> { | ||
| // Confirms that literals used before and after a join but for different columns | ||
| // are correctly handled. | ||
|
|
||
| // File generated with substrait-java's Isthmus: | ||
| // ./isthmus-cli/build/graal/isthmus --create "create table A (a int); create table B (a int, c int); create table C (a int, d int)" "select t.*, C.d, CAST(NULL AS VARCHAR) as e from (select a, CAST(NULL AS VARCHAR) as c from A UNION ALL select a, c from B) t LEFT JOIN C ON t.a = C.a" | ||
| let proto_plan = | ||
| read_json("tests/testdata/test_plans/null_literals.substrait.json"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: I would suggest naming the test and test file based on the property that we're trying to test, which is disambiguation of duplicate literal names in plans, instead of the contents of the file. A plan named
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to |
||
| let ctx = add_plan_schemas_to_ctx(SessionContext::new(), &proto_plan)?; | ||
| let plan = from_substrait_plan(&ctx.state(), &proto_plan).await?; | ||
|
|
||
| let mut settings = insta::Settings::clone_current(); | ||
| settings.add_filter( | ||
| r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}", | ||
| "[UUID]", | ||
| ); | ||
| settings.bind(|| { | ||
| assert_snapshot!( | ||
| plan, | ||
| @r#" | ||
| Projection: left.A, left.[UUID] AS C, right.D, Utf8(NULL) AS [UUID] AS E | ||
| Left Join: left.A = right.A | ||
| SubqueryAlias: left | ||
| Union | ||
| Projection: A.A, Utf8(NULL) AS [UUID] | ||
| TableScan: A | ||
| Projection: B.A, CAST(B.C AS Utf8) | ||
| TableScan: B | ||
| SubqueryAlias: right | ||
| TableScan: C | ||
| "# | ||
| ); | ||
| }); | ||
|
|
||
| // Trigger execution to ensure plan validity | ||
| DataFrame::new(ctx.state(), plan).show().await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn non_nullable_lists() -> Result<()> { | ||
| // DataFusion's Substrait consumer treats all lists as nullable, even if the Substrait plan specifies them as non-nullable. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what context does "substrait removes aliases"? When converting from Datafusion to Substrait? This is a bit a of a weird statement to me because Substrait doesn't care about names at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe my phrasing isn't best here but whatever method you use to construct the substrait, it removes column names and aliases from within the plan which means literals columns are just assigned their default names (which I think come from arrow? I didn't get as far as finding where the UTF8(NULL) name comes from)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah, I figured there was some sort of default name thing going on. Another way to phrase this might be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated