Gabriel39 commented on PR #68161: URL: https://github.com/apache/doris/pull/68161#issuecomment-5756947723
Scope and maintainability follow-up: Most of the file count is justified by regression coverage: 16 files are the two evolved Paimon table fixtures. Of the 633 added lines, 192 are production code and 438 are tests, fixture metadata, and fixture initialization/license support. Covering V1 native, V2 native, and JNI is necessary because these routes have different conversion semantics. Predicate gating and required-to-nullable handling are also part of correctness, not optional cleanup. I would still tighten this PR in three places: 1. Please split the `nonConcurrent` changes in `iceberg_and_internal_nested_namespace.groovy` and `test_iceberg_v3_row_lineage_complex_query.groovy` into a separate test-stability PR. They address a shared Iceberg namespace race, independently of Paimon timestamp precision evolution. The added Paimon fixture copy in the Iceberg Docker initializer is different: that environment supplies the MinIO warehouse used by the Paimon regression, so it belongs with the test setup. 2. Please constrain or explicitly justify the V1 behavior change. `ColumnTypeConverter::get_converter()` now truncates every same-primitive DATETIMEV2/TIMESTAMPTZ narrowing conversion, with no Paimon discriminator; the associated Parquet/ORC pushdown checks are also generic. V2 explicitly opts Paimon into the new semantics. Shared V1 readers consequently receive a broader behavior change than the PR's stated Paimon fix. Either carry an explicit table-semantics policy through V1, or document the broader contract and add non-Paimon compatibility coverage. This is a scope concern; I have not established a concrete non-Paimon regression. 3. Consider sharing the small precision predicate and fractional-truncation primitive: `requires_datetimev2_precision_conversion()` and `can_truncate_datetimev2_precision()` duplicate the same test, and the V1 converter/V2 cast separately implement the divisor and timestamp-type dispatch. Keep their distinct column ownership/nullability handling and reader boundaries; avoid introducing a large abstraction merely to reduce line count. I would retain the real historical-schema fixtures and the three-reader test matrix. Shrinking this to a getter-only fix would leave native filtering and V2 rounding incorrect; the useful simplification is to remove unrelated changes and clarify the conversion policy. -- 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]
