dongjoon-hyun commented on PR #58153: URL: https://github.com/apache/spark/pull/58153#issuecomment-5785133532
Thank you for working on this, @anuragmantri. I have a few additional comments. 1. **Error class of `WRITE_ORDERING_WITH_UNKNOWN_COLUMN`**: It is placed under `UNSUPPORTED_FEATURE` (SQLSTATE `0A000`), but the error is about a reference to a non-existent column rather than an unsupported feature. A `42703`-family condition looks more appropriate. It seems to follow `PARTITION_WITH_NESTED_COLUMN_IS_UNSUPPORTED`, but that condition was originally about nested columns, so its semantics differ. Since error classes are hard to change after release, could we decide this before merging? 2. **Inconsistent case handling**: `PreprocessTableCreation` normalizes references only for `RewritableTransform`, so an `ApplyTransform` such as `truncate(4, ID)` reaches the case-sensitive `findNestedField` check in `CheckAnalysis` as-is. As a result, even under the default case-insensitive analysis, `ORDERED BY truncate(4, ID)` fails against a column `id` while `ORDERED BY ID` succeeds. `PARTITIONED BY` has the same pre-existing limitation, so this is not a regression, but it would be good to document it or pin it with a test. 3. **Source compatibility for downstream projects**: New fields are added to the `CreateTable`, `ReplaceTable`, `CreateTableAsSelect`, and `ReplaceTableAsSelect` case classes and several Exec case classes. External extensions that pattern-match on these nodes (e.g. Delta, Iceberg Spark extensions) will fail to compile. This is acceptable because they are internal APIs, but could you mention it in the PR description? 4. **Code comments**: Some comments describe the review history, e.g. "Reusing checkTransformDuplication here was wrong" and the 13-line normalization explanation in `rules.scala`. Some are also much more verbose than the surrounding code, e.g. "This is the only thing standing between the user and that silent drop" in `WriteDistributionAndOrdering`. Could you trim them to briefly describe only the current 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: [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]
