[refactor](arrow-flight) Extract the Doris-to-Arrow type mapping into DorisArrowTypeMapping - #68315
Open
morningman wants to merge 3 commits into
Open
morningman wants to merge 3 commits into
morningman wants to merge 3 commits into
Conversation
…, one row per PrimitiveType The schema in the table_schema column of Flight SQL GetTables is the only statement FE makes about the Arrow type of a column, and a client that types its columns from it reads the batches BE emits as that type. Until now only the nested-type rules and DATEV2 were covered; the scalar table itself, including the cells known to disagree with BE (TIMESTAMPTZ with a literal "UTC" zone, Null for TIMEV2 / VARBINARY / AGG_STATE, see apache#67577), could change without any test noticing. This records the table exactly as it is, one row per PrimitiveType plus one per precision / scale band where the band picks the Arrow unit, and fails when a PrimitiveType has no row, so a new type gets a deliberate mapping rather than the default. It is the baseline for extracting the mapping into a class of its own with no value changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… DorisArrowTypeMapping FE is about to hand clients more Arrow schemas than the one in GetTables: the result and parameter schemas of prepared statements, ExecuteSchema. Each of them must describe a column exactly as the others do, or a client that types its columns from one of them fails to read the batches another promised. Until now the only mapping lived as private methods of FlightSqlSchemaHelper, where a second caller could only copy it. This moves getArrowType, the column-descriptor overload and the field builder with its nested-type rules into org.apache.doris.arrow. DorisArrowTypeMapping, unchanged: the switch, the children rules and the Flight SQL column metadata are byte for byte what they were, and the table-driven test recorded in the previous commit passes against the new class with no row touched. The cells known to disagree with BE (the literal "UTC" zone on TIMESTAMPTZ, Null for TIMEV2 / VARBINARY / AGG_STATE) are carried over as they are: they are fixed in one step against a golden shared with BE once the BE Arrow type layer has been reworked, not one call site at a time (apache#67577). FlightSqlSchemaHelper keeps the GetTables plumbing and the schema serialization; the tests split the same way, the mapping and nesting cases into DorisArrowTypeMappingTest, the serialized-schema round trips into FlightSqlSchemaHelperSerializedSchemaTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s, one table of every type DorisArrowTypeMappingTest records the mapping as a Java table; this records what reaches a client. A raw Flight SQL client asks GetTables(include_schema) for three tables that together declare a column of every type an internal table can hold -- the scalars, DATETIME and TIMESTAMPTZ at three scales each, TIMESTAMP_NS, DECIMAL(9,2) / (18,4) / (38,10) / (76,20), JSON, VARIANT, IPV4 / IPV6, BITMAP / HLL / QUANTILE_STATE / AGG_STATE, ARRAY / MAP / STRUCT and a three-level nesting -- decodes each table_schema the way a client does and compares it, field by field and down to the leaves, with the Arrow type, the nullability and the Flight SQL column metadata as they are served today. The path it covers is the whole of it: describeTables' descriptors, DorisArrowTypeMapping, the schema serialization. The cells known to disagree with BE are pinned as they are and say so: the literal "UTC" zone on TIMESTAMPTZ and Null for AGG_STATE (TIMEV2 and VARBINARY cannot be declared on an internal table; the unit test pins those). Correcting them is one deliberate step in DorisArrowTypeMapping once the BE Arrow type layer has been reworked (apache#67577), and this suite is what makes a stray change to any of them visible in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
30 tasks
Contributor
TPC-H: Total hot run time: 27876 ms |
Contributor
TPC-DS: Total hot run time: 152680 ms |
Contributor
ClickBench: Total hot run time: 23.87 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: #67577
Related PR: #66344 (the
GetTablesschema fix whose nested-type tests move here), #67966 / #68266 (the Flight SQL session work the next interfaces build on)Problem Summary:
Context. Doris speaks two client protocols: MySQL and Arrow Flight SQL (the Flight SQL JDBC driver, the ADBC drivers, Python clients). Over Flight the rows come as Arrow batches produced by BE, and
convert_to_arrow_typeinbe/src/format/arrow/arrow_row_batch.cppdecides which Arrow type each Doris type has on the wire.Besides data, Flight SQL has metadata commands.
GetTables(include_schema = true)is the client asking "which columns does this table have, and what Arrow type is each one?", and FE answers with a serialized Arrow schema in thetable_schemacolumn. Clients trust that schema: the ADBC driver types its columns from it and then decodes the batches of later queries as those types. So what FE says has to match what BE sends; a mismatch is not a degraded answer but a failed read (DATEV2 described as date64 while BE emitted date32 broke exactly that way, fixed in #66344).For that FE keeps a Doris-to-Arrow mapping, which until now was a private method of
FlightSqlSchemaHelper, the class that servesGetTables:getArrowType, plus thebuildField/arrowChildrenpair that builds a field and its nested children.1. The problem, and what it cost
ExecuteSchema. They ask the same question, "what Arrow type is this Doris type", but the answer was a private method of another class, so each of them could only copy the switch. Copies drift: it already happened once between FE and BE ([feature](timestamp_ns) Add end-to-end TIMESTAMP_NS support #66761 added TIMESTAMP_NS as one line on each side), and another copy inside FE would letGetTablesdescribe a column as one type and a prepared statement as another."UTC"(BE stamps the session time zone), and TIMEV2 / VARBINARY / AGG_STATE come back asNull(BE emits float64 / binary / binary). They are kept because the BE Arrow type layer is being reworked by another team and changing the mapping now would collide with that, so the correction is scheduled as one step after it (recorded in [Tracking] Protocol-agnostic session and execution layer: MySQL and Arrow Flight SQL as equal front ends #67577). Without a test pinning them, nothing stopped a well-meant one-cell "fix" that would change behaviour piecemeal and out of step.2. What this PR does, and why it helps
The mapping moves into a class of its own,
org.apache.doris.arrow.DorisArrowTypeMapping, the one place in FE that maps a Doris type to an Arrow type, with no value changed. Three commits:PrimitiveType(41 values; the types whose unit depends on the scale get a row per band, 47 rows in all), the known-wrong cells pinned as they are and marked "kept as is ([Tracking] Protocol-agnostic session and execution layer: MySQL and Arrow Flight SQL as equal front ends #67577)", plus a check that everyPrimitiveTypehas a row. Green against the old private method.arrow_flight_sql_p0/test_get_tables_schema: a raw Flight SQL client asks a live cluster forGetTablesof a table that declares a column of every type (three-level nesting, BITMAP / HLL / AGG_STATE, DECIMAL256 included) and pins, field by field, the type, the nullability and the column metadata a client sees. It covers what the unit test cannot: the whole path fromdescribeTables' descriptors through the mapping to the serialization and the client's decoding.What it buys:
PrimitiveTypewithout a mapping fails the test instead of silently falling intodefault -> Null.GetTablesreturns the same bytes. Checked by dumping every field of theGetTablesresult with pyarrow from an FE built before this PR and from one built at its head: 52 lines, identical.3. The classes, and how they call each other
DorisFlightSqlProducer(existing): the Flight SQL server; onCommandGetTables,getStreamTablescreates aFlightSqlSchemaHelper.FlightSqlSchemaHelper(existing, slimmed): theGetTablesplumbing only. It lists databases and tables and callsdescribeTablesthroughFrontendServiceImplto get each column'sTColumnDesc(a thrift descriptor with precision, scale and nested children), hands each column to the mapping for an ArrowField, andgetSerializedSchemawrites the fields as Arrow IPC bytes intotable_schema.DorisArrowTypeMapping(new):toArrowType(PrimitiveType, precision, scale): the table itself (the switch), a mirror of BE'sconvert_to_arrow_type.toArrowType(TColumnDesc): reads precision and scale off the descriptor and calls the above.toField(db, table, TColumnDesc): builds theField: the type, the nullability, the Flight SQL column metadata a JDBCResultSetMetaDatareads (TYPE_NAME/PRECISION/SCALE/SCHEMA_NAME/TABLE_NAME...), and the children, recursively (an ARRAY'sitem, a MAP'sentries<key, value>with the key forced non-nullable, a STRUCT's fields).FrontendServiceImpl.getColumnDesc(existing, untouched): turns a catalogColumninto aTColumnDesc.convert_to_arrow_type(untouched): the sole authority on the shape of the data on the wire; FE's table mirrors it.DorisArrowTypeMappingTest(table + nesting),FlightSqlSchemaHelperSerializedSchemaTest(serialization round trips), regressionarrow_flight_sql_p0/test_get_tables_schema.Not touched, so nobody goes looking: BE is unchanged, and FE-side results (
SHOW ..., all utf8 inFlightSqlChannel) are unchanged too; that is another recorded item that also waits for the BE rework.Release note
None
Check List (For Author)
Test
Unit tests.
DorisArrowTypeMappingTest: 47 table rows (everyPrimitiveType; DATETIMEV2 at scales 0 / 1 / 3 / 4 / 6, TIMESTAMPTZ at 0 / 3 / 6, DECIMALV2 with a declared precision it ignores), the every-type-has-a-row check, and the seven nested-type cases moved fromFlightSqlSchemaHelperArrowTypeTest;FlightSqlSchemaHelperSerializedSchemaTest: the two round trips. 57 cases, the same 57 the first commit runs against the old code.Regression.
arrow_flight_sql_p0/test_get_tables_schema(new): the scalars, DATETIME and TIMESTAMPTZ at three scales each, TIMESTAMP_NS, DECIMAL(9,2) / (18,4) / (38,10) / (76,20), JSON, VARIANT, IPV4 / IPV6, BITMAP / HLL / QUANTILE_STATE / AGG_STATE, ARRAY / MAP / STRUCT and a three-level nesting, one expected line per field; it fails on a single changed cell (checked by breaking one on purpose). Wholearrow_flight_sql_p0green locally against an FE built from this branch (14 suites, 0 failed);test_selectalso readsDatabaseMetaData.getColumns, which isGetTables(include_schema)through the Flight SQL JDBC driver.Manual. The same
GetTables(include_schema = true)dump (type, nullability, metadata, children of every field) taken over raw Flight with pyarrow from an FE built before this PR and from one built at its head: identical, 52 lines each. The suite's expected blocks are that dump.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)