924060929 commented on PR #68786: URL: https://github.com/apache/doris/pull/68786#issuecomment-6078406156
FE architecture review of `38eeebc`: An external column participates in scanning, expression evaluation, partition pruning, and writing. Preserving BINARY as VARBINARY and instant timestamps as TIMESTAMPTZ therefore needs a consistent contract across all of those paths. The overall direction is sound: connectors define remote semantics, FE handles common expression/planning rules, and connectors handle remote representation and driver differences. The partition SPI change is particularly appropriate: `bucket(binary)` produces an integer partition value, so carrying the transformed value type is necessary. Making FE pruning depend on the resolved snapshot also avoids comparing historical/transformed partition values as source-column values. The catalog migration placement is reasonable as well: reuse the ALTER journal, journal before applying, and close detached plugins after releasing the catalog lock. I would address three remaining boundaries before merging. These explain several existing bug threads; I am not raising duplicate findings: 1. **Connector-specific write conversion.** Iceberg UUID is exposed as VARBINARY(16), but `BindSink.getConnectorColumnToOutput` only sees the Doris type and performs a generic string-to-bytes cast. UUID text consequently becomes 36 bytes, while the static-partition guard rejects the same text earlier. Keep generic column binding/coercion in FE, but preserve enough remote type information in the connector write contract to normalize UUID input consistently for ordinary rows and static row/partition values. Avoid an Iceberg-specific branch in generic `BindSink`, or UUID parsing in the generic STRING-to-VARBINARY cast. See [the UUID thread](https://github.com/apache/doris/pull/68786#discussion_r4227529229). 2. **Function legality versus type coercion.** The function-name denylist in `TypeCoercionUtils` mixes choosing a common type with checking whether an execution implementation supports that type. Generic ANY signatures do not guarantee VARBINARY support; the existing map/aggregate omissions demonstrate this. Prefer function legality/signature checks, with shared checks for related functions, rather than extending this central denylist for every missed name. Analysis-time rejection improves the failure boundary, but restoring previously working queries still requires supported execution implementations. See [map membership](https://github.com/apache/doris/pull/68786#discussion_r4227529258), [min/max aggregates](https://github.com/apache/doris/pull/68786#discussion_r4227529262), and [group-array aggregates](https://github.com/apache/doris/pull/68786#discussion_r4227529266). 3. **Reader selection and capability validation.** In Paimon, raw-file convertibility, SDK raw-reader eligibility, and physical metadata availability are distinct. The reason for falling back (`legacyOrcTimestamp`) should not itself authorize metadata columns. Select the actual reader and validate its timestamp, merge, and physical-metadata capabilities together. This can be a connector-local decision; it does not require expanding the general SPI. See [the historical primary-key file thread](https://github.com/apache/doris/pull/68786#discussion_r4228357021). The JDBC dialect projections belong in the JDBC connector, but projection and Java decoding must be one read contract, including NULL/zero-date policy. The [MySQL zero-TIMESTAMP thread](https://github.com/apache/doris/pull/68786#discussion_r4227529236) is a concrete gap in that contract. The [TVF delimiter/comment issue](https://github.com/apache/doris/pull/68786#discussion_r4228357016) also remains a local fix. Keeping timezone-sensitive casts and their LIMIT local is a reasonable correctness safeguard for the existing converter that strips CAST. A richer connector expression representation retaining cast semantics would be a useful follow-up, but should not turn this PR into a general pushdown redesign. My recommendation is to retain the overall design and fix these boundaries and the linked regressions before merging. This assessment is based on source inspection and the Paimon 1.4.2 reader eligibility bytecode; I did not build or run tests. -- 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]
