You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "LiaCastaneda (via GitHub)" <gi...@apache.org> on 2025/03/26 12:55:06 UTC
[PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
LiaCastaneda opened a new pull request, #15438:
URL: https://github.com/apache/datafusion/pull/15438
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2014089960
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
If this check is skipped the query will still work, same as it can be skipped [here](https://github.com/DataDog/datafusion/blob/7299d0e566caa1e10f47a74b8ae817b6fb146fdf/datafusion/core/src/physical_planner.rs#L654) for aggregate nodes schema check. Without this we would get the error: Input field name count(Int64(1)) does not match with the projection expression count(Int64(1)):1 I think we can either skip it or adjust the projection schema names as its currently done, instead of erroing out.
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2022571200
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1470,17 +1470,27 @@ impl ValuesFields {
pub fn change_redundant_column(fields: &Fields) -> Vec<Field> {
Review Comment:
yep that's a good idea 👍
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025623252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check. I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to expect `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` and the that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` based on the logical `input_schema` and not the physical `input_exec` hence why it fails later on the check.
Could the solution rely in looking where is count(Int64(1)):1 being changed to count(Int64(1)) in the physical planner?
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025001167
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
I think we should revert this change. This check was helpful in catching many errors, especially while developing projection-related code (e.g. projection pushdown). Sorry for my delayed response, but @LiaCastaneda, could you please address the root cause of the issue and revert this change?
As you mentioned, the problem likely stems from inconsistent naming conventions between columns and fields. I recall encountering similar issues with aggregation functions in the past, and we resolved them by unifying the naming. I believe the correct fix shouldn’t require too much effort.
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2018708072
##########
datafusion/substrait/tests/cases/consumer_integration.rs:
##########
@@ -483,6 +483,34 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn test_multiple_joins() -> Result<()> {
+ let plan_str = test_plan_to_string("multiple_joins.json").await?;
+ println!("{}", plan_str);
Review Comment:
We should be fine to remove this `println!` statement now
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1470,17 +1470,27 @@ impl ValuesFields {
pub fn change_redundant_column(fields: &Fields) -> Vec<Field> {
Review Comment:
This same file has a unit test for this specific function, it would be nice to extend it to something like this:
<details><summary>change_redundant_column unit test</summary>
```rust
#[test]
fn test_change_redundant_column() -> Result<()> {
let t1_field_1 = Field::new("a", DataType::Int32, false);
let t2_field_1 = Field::new("a", DataType::Int32, false);
let t2_field_3 = Field::new("a", DataType::Int32, false);
let t1_field_2 = Field::new("b", DataType::Int32, false);
let t2_field_2 = Field::new("b", DataType::Int32, false);
let t2_field_4 = Field::new("a:1", DataType::Int32, false); // <- add this
let field_vec = vec![
t1_field_1, t2_field_1, t1_field_2, t2_field_2, t2_field_3, t2_field_4,
];
let remove_redundant = change_redundant_column(&Fields::from(field_vec));
assert_eq!(
remove_redundant,
vec![
Field::new("a", DataType::Int32, false),
Field::new("a:1", DataType::Int32, false),
Field::new("b", DataType::Int32, false),
Field::new("b:1", DataType::Int32, false),
Field::new("a:2", DataType::Int32, false),
Field::new("a:1:1", DataType::Int32, false), // <- and expect this
]
);
Ok(())
}
```
</summary>
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1470,17 +1470,27 @@ impl ValuesFields {
pub fn change_redundant_column(fields: &Fields) -> Vec<Field> {
let mut name_map = HashMap::new();
+ let mut seen: HashSet<String> = HashSet::new();
Review Comment:
At first sight, it's a bit unclear why `name_map` is not sufficient for granting uniqueness the generated names. It could be useful for readers to leave a comment here with a brief explanation about why this is needed, something like
```rust
// `name_map` tracks a mapping between a field name and the amount
// of times it appeared. If a field appeared twice with the same name, the
// count for that field will increment by one, and that count will be appended
// to the field name in order to grant uniqueness.
//
// Some field names might already come to this function with a literal appended
// count in its name, so there's still a chance of name collisions, for example,
// if these three fields passed to this function: "col", "col" and "col:1".
//
// The `seen` HashSet accounts for this, tracking the produced fields are
// truly unique.
```
Just an idea from what I understood, maybe there's a better way of explaining it
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2014089960
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
If this check is skipped the query will still work, same as it can be skipped [here](https://github.com/DataDog/datafusion/blob/7299d0e566caa1e10f47a74b8ae817b6fb146fdf/datafusion/core/src/physical_planner.rs#L654) for aggregate nodes schema check. Without this we would get the `error: Input field name count(Int64(1)) does not match with the projection expression count(Int64(1)):1 ` still it would be nice to know if this is he correct approach
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2014089960
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
If this check is skipped the query will still work, same as it can be skipped [here](https://github.com/DataDog/datafusion/blob/7299d0e566caa1e10f47a74b8ae817b6fb146fdf/datafusion/core/src/physical_planner.rs#L654) for aggregate nodes schema check. Without this we would get the error: Input field name count(Int64(1)) does not match with the projection expression count(Int64(1)):1
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same names as the projection column expressions, both have count(Int64(1)):1 so I don't know where that field is being set to count(Int64(1)) for the physcial input_schema. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as count(Int64(1)):1 . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) Expr for the projection based on the logical input_schema and not the physical input_exec schema hence why it fails later on the check.
I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the session_state to skip it (and by default set it to false), same as it is done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes. I was looking into your PR that adds that check also, but apparently we don't have `create_physical_name` anymore after #11977
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` for the projection based on the logical `input_schema` and not the physical `input_exec` schema hence why it fails later on the check.
edit: I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the `session_state` to skip it (and by default set it to false), same as it is done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025002224
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
Review Comment:
sorry for being late :( https://github.com/apache/datafusion/pull/15438/files#discussion_r2025001167
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025623252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` for the projection based on the logical `input_schema` and not the physical `input_exec` schema hence why it fails later on the check.
edit: I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the `session_state` to skip it (and by default set it to false), same as it is done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025623252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` for the projection based on the logical `input_schema` and not the physical `input_exec` hence why it fails later on the check.
Could the solution rely in looking where is count(Int64(1)):1 being changed to count(Int64(1)) in the physical planner?
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same names as the projection column expressions, both have count(Int64(1)):1 so I don't know where that field is being set to count(Int64(1)) for the physcial input_schema. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as count(Int64(1)):1 . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) Expr for the projection based on the logical input_schema and not the physical input_exec schema hence why it fails later on the check.
I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the session_state to skip it (and by default set it to false), same as it is done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes. I was looking into your PR that adds that check, but apparently we don't have `create_physical_name` anymore after #11977
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same names as the projection column expressions, both have count(Int64(1)):1 so I don't know where that field is being set to count(Int64(1)) for the physcial input_schema. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as count(Int64(1)):1 . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) Expr for the projection based on the logical input_schema and not the physical input_exec schema hence why it fails later on the check.
I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the session_state to skip it (and by default set it to false) so errors can still be caught while developing (iiuc this doesn't cause errors during execution), something similar was done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes. I was looking into your PR that adds that check, but apparently we don't have `create_physical_name` anymore after #11977
edit: I have a solution in mind based on what I mentioned above, can this wait a couple of days instead of reverting the PR commit, since it will also revert the duplicate schema names fix. I will try opening another PR this week which will include the check back.
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same names as the projection column expressions, both have count(Int64(1)):1 so I don't know where that field is being set to count(Int64(1)) for the physcial input_schema. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as count(Int64(1)):1 . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) Expr for the projection based on the logical input_schema and not the physical input_exec schema hence why it fails later on the check.
I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the session_state to skip it (and by default set it to false) so errors can still be caught while developing (iiuc this doesn't cause errors during execution), something similar was done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes. I was looking into your PR that adds that check, but apparently we don't have `create_physical_name` anymore after #11977
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#issuecomment-2771897735
Thanks for the reviews @alamb and @gabotechs
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025623252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` and the that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` based on the logical `input_schema` and not the physical `input_exec` hence why it fails later on the check.
Could the solution rely in looking where is count(Int64(1)):1 being changed to count(Int64(1)) in the physical planner?
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2029248877
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
https://github.com/apache/datafusion/pull/15580
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#issuecomment-2770491240
Hi @LiaCastaneda -- I believe the CI has failed on this PR due to a change in the CI actions. Can you please merge the PR up to main which i think will address the issue
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025623252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same names as the projection column expressions, both have `count(Int64(1)):1` so I don't know where that field is being set to `count(Int64(1))` for the physcial `input_schema`. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as `count(Int64(1)):1` . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) `Expr` for the projection based on the logical `input_schema` and not the physical `input_exec` schema hence why it fails later on the check.
Could the solution rely in looking where is count(Int64(1)):1 being changed to count(Int64(1)) in the physical planner?
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15438:
URL: https://github.com/apache/datafusion/pull/15438
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same names as the projection column expressions, both have count(Int64(1)):1 so I don't know where that field is being set to count(Int64(1)) for the physcial input_schema. On the physical planner I also printed the Logical input schema [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009) and it appears as count(Int64(1)):1 . I also noticed that same function [creates a physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042) Expr for the projection based on the logical input_schema and not the physical input_exec schema hence why it fails later on the check.
I think an approach could be to move the check [here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028) and add an option on the runtime config we can get thorugh the session_state to skip it (and by default set it to false) so errors can still be caught while developing (iiuc this doesn't cause errors during execution), something similar was done for [aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658) nodes. I was looking into your PR that adds that check, but apparently we don't have `create_physical_name` anymore after #11977
edit: I have a solution in mind based on what I mentioned above, can this wait a couple of days instead of reverting the PR commit? I will try opening the fix PR this week which will include the check back.
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2014087709
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1470,17 +1470,26 @@ impl ValuesFields {
pub fn change_redundant_column(fields: &Fields) -> Vec<Field> {
let mut name_map = HashMap::new();
+ let mut seen: HashSet<String> = HashSet::new();
+
fields
.into_iter()
.map(|field| {
- let counter = name_map.entry(field.name().to_string()).or_insert(0);
- *counter += 1;
- if *counter > 1 {
- let new_name = format!("{}:{}", field.name(), *counter - 1);
- Field::new(new_name, field.data_type().clone(), field.is_nullable())
- } else {
- field.as_ref().clone()
Review Comment:
This seems to be the root cause of the issue: when doing joins, there is a function [requalify_sides_if_needed](https://github.com/DataDog/datafusion/blob/7299d0e566caa1e10f47a74b8ae817b6fb146fdf/datafusion/substrait/src/logical_plan/consumer.rs#L1795) to handle aliasing the columns so the resulting schema of a join : `let in_join_schema = left.schema().join(right.schema())?;` can be created . However, if we had a query like:
```
select
*
from
first_agg
LEFT JOIN fourth_random_table ON first_agg.id = fourth_random_table.id
LEFT JOIN second_agg ON first_agg.id = second_agg.id
LEFT JOIN third_agg ON first_agg.id = third_agg.id
```
The first JOIN to be converted to a logical plan: `LEFT JOIN third_agg ON first_agg.id = third_agg.id` will work, the join schema col names will stay as they are with an alias , however on the subsequent JOINs it will fail since the consumer does the following steps for each JOIN:
1. After handling the innermost join the resulting join schema is `[left.id] [right.id]` ✅
2. For the second join it we ["carry" the previous schema](https://github.com/DataDog/datafusion/blob/7299d0e566caa1e10f47a74b8ae817b6fb146fdf/datafusion/substrait/src/logical_plan/consumer.rs#L1262), so in `requalify_sides_if_needed` we would have `[id, left.id] [id, right.id]` so we would have to alias again -> `[left.id, left.id] [right.id, right.id]` and because of this function we would end up having: `[left.id:1 , left.id] [right.id:1 , right.id]` ✅
3. On the outermost and final join the process would be repeated: `[id, left.id:1 , left.id] [id, right.id:1 , right.id]` ->
`[left.id:1, left.id:1 , left.id] [right.id:1, right.id:1 , right.id]` and because of id:1 being repeated with the current `change_redundant_column` algorithm, the query will fail with `Schema contains duplicate unqualified field name "id:1"` 🟥
Moreover we can observe that if we do just two levels of joins we would get no error:
```
select
*
from
first_agg
LEFT JOIN fourth_random_table ON first_agg.id = fourth_random_table.id
LEFT JOIN second_agg ON first_agg.id = second_agg.id
```
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#issuecomment-2768931890
👋 Hi @alamb I was wondering if I could get a review on this PR, it happens while using the substrait consumer but it fixes a potential issue you observed [here](https://github.com/apache/datafusion/pull/9154#discussion_r1492342028) 🙇♀️
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2029248877
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
https://github.com/apache/datafusion/pull/15580
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2029264252
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
#15580
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#issuecomment-2771896879
Thanks for the review @alamb
--
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: [PR] Fix duplicate unqualified Field name (schema error) on join queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2023216031
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
Review Comment:
FYI @berkaysynnada and @akurmustafa
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1468,19 +1468,37 @@ impl ValuesFields {
}
}
+// `name_map` tracks a mapping between a field name and the number of appearances of that field.
+//
+// Some field names might already come to this function with the count (number of times it appeared)
+// as a sufix e.g. id:1, so there's still a chance of name collisions, for example,
+// if these three fields passed to this function: "col:1", "col" and "col", the function
+// would rename them to -> col:1, col, col:1 causing a posteriror error when building the DFSchema.
+// that's why we need the `seen` set, so the fields are always unique.
+//
pub fn change_redundant_column(fields: &Fields) -> Vec<Field> {
let mut name_map = HashMap::new();
+ let mut seen: HashSet<String> = HashSet::new();
+
fields
.into_iter()
.map(|field| {
- let counter = name_map.entry(field.name().to_string()).or_insert(0);
- *counter += 1;
- if *counter > 1 {
- let new_name = format!("{}:{}", field.name(), *counter - 1);
- Field::new(new_name, field.data_type().clone(), field.is_nullable())
- } else {
- field.as_ref().clone()
+ let base_name = field.name();
+ let count = name_map.entry(base_name.clone()).or_insert(0);
+ let mut new_name = base_name.clone();
Review Comment:
I played around with trying to avoid this `clone`, but I could not come up with anything that was reasonable
Since this function is only called when creating subqueries I think it is fine
https://github.com/search?q=repo%3Aapache%2Fdatafusion%20change_redundant_column&type=code
--
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