kosiew commented on code in PR #23226:
URL: https://github.com/apache/datafusion/pull/23226#discussion_r3534208754
##########
datafusion/catalog-listing/src/helpers.rs:
##########
Review Comment:
I think the new decoded partition semantics can still be bypassed by prefix
pruning here.
`evaluate_partition_prefix` builds the object-store prefix from the SQL
literal directly with `parts.push(format!(\"{p}={val}\"))`, but this PR makes
the SQL-visible value decoded. For a file like
`category=Electronics%2FComputers/data.parquet`, a query such as `category =
'Electronics/Computers'` would list the prefix `category=Electronics/Computers`
instead of the encoded path segment. That means the file is never seen, so the
later decoded comparison cannot match it.
Could we either encode partition values when constructing listing prefixes,
or disable exact prefix pruning for values that need escaping? It would also be
great to add a regression test for an equality filter on a decoded value
containing `/` or a space.
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -525,7 +536,7 @@ mod tests {
#[test]
fn test_parse_partitions_for_path() {
Review Comment:
Nice coverage here. One small suggestion: consider turning the added
encoded-value assertions in `test_parse_partitions_for_path` into a small table
of `(path, column, expected)` cases. That would make future Hive escaping cases
easier to add without growing one long assertion block.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
