morningman commented on PR #67530: URL: https://github.com/apache/doris/pull/67530#issuecomment-5550561389
@yiguolei Sure. Before you start, here is what we need from that refactor, so that our follow-up work can build on it instead of fighting it. Context: we are preparing a proposal that makes MySQL and Arrow Flight SQL equal front ends over one session layer, and its type-mapping part depends on exactly the code this PR touches (`convert_to_arrow_type`, `create_arrow_field_with_metadata` and the schema entry points in `arrow_row_batch.cpp`). 1. **One enumerable mapping, table-testable.** Whatever shape the refactor takes (per-`DataType` virtuals, serde methods, a registry), please keep a single place that answers "Doris type -> Arrow storage type + field metadata" for every type, so that a table-driven BE UT can walk all types and the FE can validate its mirror (`FlightSqlSchemaHelper.getArrowType`, to be extracted) against the same golden file. Today the FE copy already disagrees with the BE in four places (TIMESTAMPTZ zone, TIMEV2, VARBINARY, AGG_STATE); we want that class of drift to be caught by a test, not by a client. 2. **Field metadata at every nesting level.** Arrow keeps metadata on `Field`, not on `DataType`, so ARRAY item / MAP key and value / STRUCT children have to be built through the metadata helper (what this PR fixes). We will add `ARROW:extension:name` next to `doris_type` for the types Arrow has no native equivalent for (LARGEINT, IPV4, IPV6, JSON, VARIANT); adding a metadata key should be a one-line change, not an edit in every branch. 3. **Storage types stay as they are.** The same functions feed the Spark/Flink connector read path (`memory_scratch_sink_operator`), Python UDF/UDAF/UDTF and the Parquet transformer, so a storage-type change is a wire-format change for connectors. Our plan is to keep the current shapes (LARGEINT -> `utf8`, IPV4 -> `int32`, IPV6 -> `utf8`, DECIMALV2 -> `decimal128(27,9)`, DATETIME -> naive `timestamp` with the unit chosen by scale, TIMESTAMPTZ with its zone) and only add metadata. If the refactor wants to change any of them, please make it an explicit, separately reviewed change rather than a side effect. 4. **Schema authoritative over batches.** A Flight stream has one schema, so a builder must never swap a column's array type per batch (`utf8` -> `large_utf8`), see #65789. If the string/binary builders are touched, keep the split-by-rows approach. 5. **AGG_STATE.** It currently goes through `get_serialized_type()` and lands in `utf8` carrying non-UTF-8 bytes; if the refactor handles it, `binary` is the right target. 6. **Keep the schema entry points** (`get_arrow_schema_from_block` / `get_arrow_schema_from_expr_ctxs`, or equivalents): the FE fetches the result schema from the BE (`fetchArrowFlightSchema`) and forwards it to clients as is. Could you share the intended scope and rough timing? We would rebase our type-mapping PRs on top of it, and it also settles whether this PR should go in first or be folded into the refactor. -- 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]
