fix: PushDownFilter for GROUP BY on uppercase col names

[aditanase]

Which issue does this PR close?

PushDownFilter does not push a predicate when the table has columns that are not all lowercase. Tried with and without enable_ident_normalization - no change. The logic inside parse_identifiers_normalized does not seem to properly detect quotes and it will lowercase the column used in the group by expression.

Here's the query I used, just for illustration:

SELECT * FROM (
    SELECT
        fm.Timestamp,
        SUM(fm.ConsumedUnits)
    FROM fm
    WHERE
        fm.Timestamp BETWEEN to_timestamp('2025-04-10') AND to_timestamp('2025-04-20')
    GROUP BY fm.Timestamp
)
WHERE Timestamp = to_timestamp('2025-04-12')

Expected query plan:

Aggregate: groupBy=[[fm.Timestamp]], aggr=[[sum(fm.ConsumedUnits)]]
  TableScan: fm projection=[ConsumedUnits, Timestamp], full_filters=[fm.Timestamp = TimestampMicrosecond(1744416000000000, Some("UTC")), fm.Timestamp >= TimestampMicrosecond(1744243200000000, Some("UTC")), fm.Timestamp <= TimestampMicrosecond(1745107200000000, Some("UTC"))]

Actual query plan:

Filter: fm.Timestamp = TimestampMicrosecond(1744416000000000, Some("UTC"))
  Aggregate: groupBy=[[fm.Timestamp]], aggr=[[sum(fm.ConsumedUnits)]]
    TableScan: fm projection=[ConsumedUnits, Timestamp], full_filters=[fm.Timestamp >= TimestampMicrosecond(1744243200000000, Some("UTC")), fm.Timestamp <= TimestampMicrosecond(1745107200000000, Some("UTC"))]

An alterate fix could use expr_to_columns to extract the columns, as in Unnest above:

let mut group_expr_columns: HashSet<Column> = HashSet::new();
for p in &agg.group_expr {
    expr_to_columns(&p, &mut group_expr_columns)?;
}

Question: should we make the same change in the Window functions branch?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes, from a client application.

I did not add any unit tests, none of the existing tests in this module use upper case columns. Tried to add another table/schema, but then the test was failing, I am unsure of how to control the lowercasing of column names.

Are there any user-facing changes?

[aditanase]

@adriangb Saw you recent commits in this area, would appreciate if you weighed in on this. Thank you! 🙌

[adriangb]

Hmm I've been doing a lot with the physical optimizer of the same name but haven't touched the logical optimizer. The recent changes may mean that the pushdown ends up happening regardless at the physical level but I think it's worth fixing the logical level anyway.

I don't fully understand the issue: does from_qualified_name_ignore_case do something different with quotes than from_qualified_name? I also don't see any quotes in your example.

[aditanase]

I don't fully understand the issue: does from_qualified_name_ignore_case do something different with quotes than from_qualified_name? I also don't see any quotes in your example.

Sorry for not being more clear. I was referring to these lines:

datafusion/datafusion/common/src/utils/mod.rs

Lines 297 to 298 in 923bfb7

	.map(|id| match id.quote_style {
	Some(_) => id.value,

Reading it made me think that if I used quotes I might convince it to remain unchanged, but it still converts to lowercase timestamp, no matter what I tried, so I didn't include it in the code sample.

[alamb]

Can we please get a test for this fix so we don't break it again in the future?

[alamb]

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

[aditanase]

@alamb Thanks for waiting, added a test that would break without this change

[xudong963]

I also suggest adding sqllogictest based on the sql in PR summary

[xudong963]

If the table also has a column named 'a', what'll happen?

[aditanase]

Great question, just tried this and it works as expected for both uppercase and lower case col, even if both are present in the schema at the same time. I added another test, lmk if we should keep it or it's overkill.

[aditanase]

I also suggest adding sqllogictest based on the sql in PR summary

I'd be happy to, can you please point me at a sample patch or a good suite to add to? Last time I tried there was quite a bit of ceremony around SLT, not sure if I can get it right on first approach. Thanks!