You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "aaraujo (via GitHub)" <gi...@apache.org> on 2025/09/17 21:51:07 UTC
[PR] fix: resolve qualified column references after aggregation [datafusion]
aaraujo opened a new pull request, #17634:
URL: https://github.com/apache/datafusion/pull/17634
Adds fallback logic to handle qualified column references when they fail to resolve directly in the schema. This commonly occurs when aggregations produce unqualified schemas but subsequent operations still reference qualified column names.
The fix preserves original error messages and only applies the fallback for qualified columns that fail initial resolution.
## Which issue does this PR close?
- Fixes a bug without an issue
## Rationale for this change
When aggregation operations produce schemas with unqualified column names, subsequent operations that reference qualified column names (e.g., `table.column`) fail to resolve even though the underlying column exists. This breaks query execution in cases where the logical plan correctly maintains qualified references but the schema resolution cannot handle the qualification mismatch.
## What changes are included in this PR?
- Modified `get_type()` and `nullable()` functions in `expr_schema.rs` to include fallback logic
- Added conservative fallback that only applies to qualified columns that fail initial resolution
- Preserves original error messages when both qualified and unqualified resolution fail
- Added comprehensive tests covering various scenarios including edge cases
## Are these changes tested?
Yes, this PR includes:
- Three new test cases covering the specific fix scenarios
- Edge case testing for unqualified columns and non-existent columns
- All existing expr tests continue to pass (134/135, with 1 pre-existing failure unrelated to this change)
- Verified with cargo fmt, clippy, and full test suite
## Are there any user-facing changes?
No breaking changes. This fix resolves previously failing queries involving qualified column references after aggregation, making the behavior more consistent and intuitive for users.
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3319370075
@zhuqi-lucas fixed a potentially related issue here:
- https://github.com/apache/datafusion/pull/17706
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "aaraujo (via GitHub)" <gi...@apache.org>.
aaraujo closed pull request #17634: fix: resolve qualified column references after aggregation
URL: https://github.com/apache/datafusion/pull/17634
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3316347839
Can you provide an actual full query? One that includes the actual aggregation itself? That example you provided doesn't seem like a valid reproduction as it isn't a full query and just seems engineered exactly to showcase this "bug".
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "aaraujo (via GitHub)" <gi...@apache.org>.
aaraujo commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3313771203
Thank you for the feedback @alamb! You raise a valid point about treating symptoms vs root causes. Let me provide additional context from where this issue was discovered during real-world integration testing that demonstrates why this fix addresses the right level of the problem.
## Real-World Context
This issue was discovered during PackDB integration testing (my time-series database project) where DataFusion is used as the query engine for PromQL queries. The specific failing pattern was:
```promql
avg(memory_usage_bytes) / 1024 / 1024
```
The Problem in Practice
1. The aggregation avg(memory_usage_bytes) produces a schema with unqualified column names
2. The subsequent binary expression (/ 1024 / 1024) still references the qualified column name from the original metric
3. This caused schema resolution failures with errors like: "No field named memory_usage_bytes.timestamp. Valid fields are value."
Impact: This affected 96.6% of dashboard queries (28/29 failing), making it a critical blocker for any system using qualified references in arithmetic operations after aggregation.
Why This is the Right Level to Fix
1. Aggregation Behavior is Correct: Aggregations should produce unqualified schemas - this is standard SQL behavior and shouldn't change.
2. Query Builder Reality: Many query builders, ORMs, and query engines (like PromQL→DataFusion translation) consistently use qualified references throughout the query pipeline. Expecting them to track and strip qualifiers after aggregations would require complex context-aware logic in every consumer.
3. Backwards Compatibility: This fix provides a graceful fallback that maintains existing behavior while supporting the common pattern of qualified references.
4. Conservative Approach: The fix only activates when qualified lookup fails, preserves original error messages, and follows the principle of "be liberal in what you accept."
Evidence of Fix Effectiveness
- PackDB now uses this DataFusion fork and dashboards work correctly
- The test case demonstrates the exact failing scenario and verifies the fix
- Integration tests confirm arithmetic operations on aggregated metrics now resolve properly
This isn't treating a symptom - it's addressing a legitimate schema resolution gap that affects real-world query patterns while maintaining all existing behavior.
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "aaraujo (via GitHub)" <gi...@apache.org>.
aaraujo commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3328688468
@alamb @Jefffrey Thank you for the thorough review feedback. After conducting an extensive investigation into the real-world impact of this issue, I need to recommend **closing this PR**. Here's what we discovered:
### How We Originally Hit This Issue
This issue was first identified during PackDB integration testing where we saw query translation failures. The initial investigation suggested that 96.6% of dashboard queries were affected by qualified column reference resolution after aggregation operations. However, a deeper analysis revealed this was a red herring.
### What Our Investigation Revealed
#### 1. **PackDB Doesn't Actually Need This Fix**
After analyzing the current PackDB codebase (which uses DataFusion 50.0.0), we discovered that PackDB naturally avoids this issue entirely through its query translation patterns:
```rust
// PackDB's actual pattern (from promql/planner_impl.rs):
.aggregate(group_by_cols, vec![aggregate_expr.alias("value")])?
.project(vec![lit(context.end).alias("timestamp"), col("value")])?
// ^^^^^^^^^
// Always unqualified
```
PackDB consistently uses unqualified column references after aggregation, which is the natural and correct DataFrame API usage pattern.
#### 2. The Fix is Incomplete
The current fix only addresses expression evaluation (get_type() and nullable() methods) but doesn't fix the main claimed use case:
```rust
// This still fails even with the fix:
let result = aggregated.select(vec![col("metrics.avg_cpu")]);
// Error: Schema error: No field named metrics.avg_cpu
// Only this works (binary expressions):
let expr = col("metrics.avg_cpu") * lit(2.0); // Fixed by this PR
```
The fix doesn't extend to field_from_column() which is used by .select() operations - the primary DataFrame API method mentioned in all examples.
#### 3. No Real-World Usage Pattern Identified
After extensive searching, we could only identify one contrived scenario where this might be needed:
```rust
// Poorly designed query builder that retains qualifiers
fn build_query(table_name: &str) -> Result<DataFrame> {
let aggregated = table.aggregate(/*...*/)?;
// Bad pattern: accidentally using qualified reference to aggregated column
aggregated.select(vec![
col(&format!("{}.avg_metric", table_name)) // This would fail
])
}
```
However, this represents poor API usage that should be discouraged, not accommodated.
#### 4. Natural Programming Patterns Avoid This
Well-designed DataFrame code naturally avoids this issue:
```rust
// Natural pattern - developers reference the columns they create
let aggregated = table.aggregate(
vec![col("region")],
vec![avg(col("cpu_usage")).alias("avg_cpu")] // Create "avg_cpu"
)?;
let result = aggregated.select(vec![
col("region"),
col("avg_cpu") // Reference what we created - no qualification needed
])?;
```
Why This Issue Appeared Initially
The original PackDB integration issues were likely caused by:
1. Early development patterns that were later refactored
2. Query translation bugs that were fixed through different means
3. Misattribution - other schema resolution issues that weren't related to qualified column references
Current Error Handling is Actually Good
The existing error message is clear and actionable:
```
Schema error: No field named metrics.avg_cpu. Did you mean 'avg_cpu'?
```
This guides users toward the correct unqualified column usage, which is the proper DataFrame API pattern.
Recommendation
I recommend closing this PR because:
1. No real-world impact: The motivating use case (PackDB) doesn't actually need this fix
2. Incomplete solution: Doesn't fix the main claimed use case (.select() operations)
3. Encourages bad patterns: Would accommodate poor API usage rather than guide users to better practices
4. No community need: No evidence of other DataFusion users hitting this specific pattern
Thank you both for the detailed review process - it helped us discover that the original problem had already been solved through better API usage patterns.
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3315463504
Could you provide a reproduction of the query that prompted this fix, so we can investigate to see if there is a root cause?
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "aaraujo (via GitHub)" <gi...@apache.org>.
aaraujo commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3316071773
@Jefffrey Absolutely! Here's a standalone test case that reproduces the issue without external dependencies:
## Standalone Reproduction
```rust
// Save as: datafusion/core/examples/qualified_column_repro.rs
// Run with: cargo run --example qualified_column_repro
use datafusion::arrow::datatypes::{DataType, Field};
use datafusion::common::{Column, DFSchema, Result, TableReference};
use datafusion::logical_expr::{lit, BinaryExpr, Expr, ExprSchemable, Operator};
#[tokio::main]
async fn main() -> Result<()> {
// Create a schema that represents the output of an aggregation
// Aggregations produce unqualified column names in their output schema
let post_agg_schema = DFSchema::from_unqualified_fields(
vec![Field::new("avg(metrics.value)", DataType::Float64, true)].into(),
Default::default(),
)?;
println!("Post-aggregation schema has field: {:?}",
post_agg_schema.fields()[0].name());
// Create a qualified column reference (as the optimizer might produce)
let qualified_col = Expr::Column(Column::new(
Some(TableReference::bare("metrics")),
"avg(metrics.value)"
));
// Create a binary expression: metrics.avg(metrics.value) / 1024
let binary_expr = Expr::BinaryExpr(BinaryExpr::new(
Box::new(qualified_col.clone()),
Operator::Divide,
Box::new(lit(1024.0)),
));
println!("\nTrying to resolve qualified column: metrics.avg(metrics.value)");
match qualified_col.get_type(&post_agg_schema) {
Ok(dtype) => println!("✓ SUCCESS: Resolved to type {:?}", dtype),
Err(e) => println!("✗ ERROR: {}", e),
}
println!("\nTrying to resolve binary expression: metrics.avg(metrics.value) / 1024");
match binary_expr.get_type(&post_agg_schema) {
Ok(dtype) => println!("✓ SUCCESS: Resolved to type {:?}", dtype),
Err(e) => println!("✗ ERROR: {}", e),
}
Ok(())
}
```
Results
Without the fix:
Post-aggregation schema has field: "avg(metrics.value)"
Trying to resolve qualified column: metrics.avg(metrics.value)
✗ ERROR: Schema error: No field named metrics."avg(metrics.value)". Did you mean 'avg(metrics.value)'?.
Trying to resolve binary expression: metrics.avg(metrics.value) / 1024
✗ ERROR: Schema error: No field named metrics."avg(metrics.value)". Did you mean 'avg(metrics.value)'?.
With the fix:
Post-aggregation schema has field: "avg(metrics.value)"
Trying to resolve qualified column: metrics.avg(metrics.value)
✓ SUCCESS: Resolved to type Float64
Trying to resolve binary expression: metrics.avg(metrics.value) / 1024
✓ SUCCESS: Resolved to type Float64
The Issue
The problem occurs when:
1. An aggregation produces an unqualified output schema (e.g., avg(metrics.value) becomes just "avg(metrics.value)" without the table qualifier)
2. Subsequent operations (like binary expressions) still reference the qualified column name (metrics."avg(metrics.value)")
3. Schema resolution fails because the qualified name doesn't exist in the post-aggregation schema
This pattern commonly occurs in query builders, ORMs, and SQL translation layers that maintain qualified references throughout the query pipeline for clarity and correctness.
The Fix
The fix adds a fallback mechanism in expr_schema.rs that:
- First attempts to resolve the column with its qualifier
- If that fails AND the column has a qualifier, tries resolving without the qualifier
- Only returns an error if both attempts fail (preserving the original error message)
This conservative approach maintains backward compatibility while enabling legitimate query patterns that were previously failing.
--
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: resolve qualified column references after aggregation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17634:
URL: https://github.com/apache/datafusion/pull/17634#issuecomment-3330210406
Glad to hear you sorted out it. Thank you @aaraujo
--
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