You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/14 15:57:18 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
alamb opened a new pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/1447
# Rationale for this change
There was a bug introduced in https://github.com/apache/arrow-datafusion/pull/1378
# What changes are included in this PR?
Do not copy the field from the input schema, but explicitly copy the metadata
# Are there any user-facing changes?
Queries that should work will once again do so
cc @hntd187
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 edited a comment on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
hntd187 edited a comment on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993772278
> Queries that should work will once again do so
I like what you did there :)
I have a perhaps naive question, shouldn't aliasing a query to an already existing column just fail outright? Wouldn't that reasonably be something of an "which column do you really want here?" confusion?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993933267
> Ahh this fails because of the count changing the type of usage_system and usage_user correct?
Yes, I think the type of `usage_system` is `UInt64` but the output of the projection `count(usage_system)` is `Int64` which is not the same as `UInt64` 🤣
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 commented on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
hntd187 commented on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993772278
> Queries that should work will once again do so
I like what you did there :)
I have a perhaps naive question, shouldn't aliasing a query to an already existing column just fail outright? Wouldn't that reasonably be something of an "which column do you really want here?" confusion?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 commented on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
hntd187 commented on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993902766
Ahh this fails because of the count changing the type of usage_system and usage_user correct?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993877514
> I have a perhaps naive question, shouldn't aliasing a query to an already existing column just fail outright? Wouldn't that reasonably be something of a "which column do you really want here?" confusion?
@hntd187 -- I agree that aliasing one column to another column is unlikely to be actually useful 😆 it was just the minimal reproducer I could come up with.
Another use of `Projection`, despite its slightly misleading name, is to evaluate expressions as well as to control the names of the fields in the output schema, which is what IOx was doing that triggered this bug.
Specifically, the plan in the IOx test was:
```
2021-12-13T14:38:49.774041Z DEBUG datafusion::execution::context: Logical plan:
Projection: #cpu.cpu, #cpu.host, CAST(#usage_system AS Int64) AS usage_system, CAST(#usage_user AS Int64) AS usage_user, #time
Sort: #cpu.cpu ASC NULLS FIRST, #cpu.host ASC NULLS FIRST
Projection: #cpu.cpu, #cpu.host, #usage_system, #usage_user, #time
Aggregate: groupBy=[[#cpu.cpu, #cpu.host]], aggr=[[COUNT(#cpu.usage_system AS usage_system) AS usage_system, COUNT(#cpu.usage_user AS usage_user) AS usage_user, MAX(#cpu.time) AS time]]
Filter: TimestampNanosecond(0) <= #cpu.time AND #cpu.time < TimestampNanosecond(2001)
TableScan: cpu projection=None
```
Which was built programatically, but is approximately what would come out of this query
```sql
SELECT
cpu,
host,
count(usage_system) as usage_system,
count(usage_user) as usage_user
max(time) as time,
FROM
cpu
WHERE
0 < time
AND time < 2001
```
In this case, the consumer of the output expect the columns named a certain way, and without the alias `count(usage_system)` results in a column named something like `count(usage_system)` rather than the expected `usage_system`, and this the alias is added
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#discussion_r768809913
##########
File path: datafusion/src/physical_plan/projection.rs
##########
@@ -63,13 +63,22 @@ impl ProjectionExec {
let fields: Result<Vec<Field>> = expr
.iter()
- .map(|(e, name)| match input_schema.field_with_name(name) {
- Ok(f) => Ok(f.clone()),
Review comment:
I totally missed this in my review -- basically the projection's output is not the same for its input even if the field name matches (due to aliases). The actual output type needs to be calculated from the expr for all cases.
##########
File path: datafusion/src/physical_plan/projection.rs
##########
@@ -63,13 +63,22 @@ impl ProjectionExec {
let fields: Result<Vec<Field>> = expr
.iter()
- .map(|(e, name)| match input_schema.field_with_name(name) {
- Ok(f) => Ok(f.clone()),
- Err(_) => {
- let dt = e.data_type(&input_schema)?;
- let nullable = e.nullable(&input_schema)?;
- Ok(Field::new(name, dt, nullable))
- }
+ .map(|(e, name)| {
Review comment:
I am not convinced it is a great idea to copy field metadata from the input to the output based on the field name alone... As that would mean metadata on a field named `a` from the input could end up on the output field `a` even if the output was not related at all to `a`.
Thus I ~will make~ made this code only copy field level metadata for a direct input column reference
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 edited a comment on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
hntd187 edited a comment on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993772278
> Queries that should work will once again do so
I like what you did there :)
I have a perhaps naive question, shouldn't aliasing a query to an already existing column just fail outright? Wouldn't that reasonably be something of a "which column do you really want here?" confusion?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#issuecomment-993877514
> I have a perhaps naive question, shouldn't aliasing a query to an already existing column just fail outright? Wouldn't that reasonably be something of a "which column do you really want here?" confusion?
@hntd187 -- I agree that aliasing one column to another column is unlikely to be actually useful 😆 it was just the minimal reproducer I could come up with.
Another use of `Projection`, despite its slightly misleading name, is to evaluate expressions as well as to control the names of the fields in the output schema, which is what IOx was doing that triggered this bug.
Specifically, the plan in the IOx test was:
```
2021-12-13T14:38:49.774041Z DEBUG datafusion::execution::context: Logical plan:
Projection: #cpu.cpu, #cpu.host, CAST(#usage_system AS Int64) AS usage_system, CAST(#usage_user AS Int64) AS usage_user, #time
Sort: #cpu.cpu ASC NULLS FIRST, #cpu.host ASC NULLS FIRST
Projection: #cpu.cpu, #cpu.host, #usage_system, #usage_user, #time
Aggregate: groupBy=[[#cpu.cpu, #cpu.host]], aggr=[[COUNT(#cpu.usage_system AS usage_system) AS usage_system, COUNT(#cpu.usage_user AS usage_user) AS usage_user, MAX(#cpu.time) AS time]]
Filter: TimestampNanosecond(0) <= #cpu.time AND #cpu.time < TimestampNanosecond(2001)
TableScan: cpu projection=None
```
Which was built programatically, but is approximately what would come out of this query
```sql
SELECT
cpu,
host,
count(usage_system) as usage_system,
count(usage_user) as usage_user
max(time) as time,
FROM
cpu
WHERE 0 < time AND time < 2001
```
In this case, the consumer of the output expect the columns named a certain way, and without the alias `count(usage_system)` results in a column named something like `count(usage_system)` rather than the expected `usage_system`, and this the alias is added
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] hntd187 commented on a change in pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448#discussion_r768857740
##########
File path: datafusion/src/physical_plan/projection.rs
##########
@@ -63,13 +63,22 @@ impl ProjectionExec {
let fields: Result<Vec<Field>> = expr
.iter()
- .map(|(e, name)| match input_schema.field_with_name(name) {
- Ok(f) => Ok(f.clone()),
- Err(_) => {
- let dt = e.data_type(&input_schema)?;
- let nullable = e.nullable(&input_schema)?;
- Ok(Field::new(name, dt, nullable))
- }
+ .map(|(e, name)| {
Review comment:
This was the question I had asked previously about when projections created new columns, basically, it doesn't seem to make sense to just blindly carry over metadata because it's about a column that may no longer exist.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1448: Fix bug in projection: "column types must match schema types, expected XXX but found YYY"
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1448:
URL: https://github.com/apache/arrow-datafusion/pull/1448
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org