MisterRaindrop commented on PR #1951: URL: https://github.com/apache/cloudberry/pull/1951#issuecomment-5645704420
Thanks for the thorough read, and for pointing at tea — its type table and its field-id path settled two of the decisions below. Pushed as c75f6e94c70; the regression suites pass on Arrow 9.0.0, and the paths the suites cannot reach (files written by pyarrow 21: dictionary-encoded, large_string, INT96, with and without field ids, invalid UTF-8) were checked by hand. One framing note first: this PR settles the framework — memory, files, how columns are matched — and every type question that still has a real choice in it goes to its own issue rather than into this diff. Item by item: **1. A failed write destroys a pre-existing file.** Done as you suggested: `open(O_CREAT|O_EXCL)`, the descriptor handed to `FileOutputStream::Open(int)`, and every failure path — including a third one at writer creation that had its own inline `unlink` — goes through `parquet_discard`, which now only ever removes a file this writer created. Writing to a path that exists is refused and the file is untouched afterwards; that is in the tests. **2. `parquet_compression`.** Done: the name is lower-cased and asked of Arrow in three steps — `Codec::GetCompressionType`, `parquet::IsCodecSupported`, `Codec::IsAvailable` — so the message says which of the three refused. `'GZIP'` is accepted, `'lzo'` is refused as a codec Parquet cannot use, `'deflate'` as one Arrow does not know. **3. Dropped columns.** Done: skipped in the schema, the batch builder and the writer, with the descriptor-to-schema position mapping kept in the builder. (`ALTER TABLE` on a lake table is refused wholesale today, so the case cannot yet be reached from SQL; it will be.) **4. Memory accounting.** Done in this PR rather than deferred. `format/arrow_memory_pool.cpp` wraps `default_memory_pool()` in a `ProxyMemoryPool` that reserves with `VmemTracker_ReserveVmem` before allocating and releases after freeing, so `gp_vmem_protect_limit`, `gp_vmem_limit_per_query` and the resource group see Arrow's memory the way they see palloc's, and a refusal surfaces as `ERRCODE_OUT_OF_MEMORY` instead of an OOM-killed segment. It is the same shape as ClickHouse's `ArrowMemoryPool` over its `MemoryTracker`; the PostgreSQL-specific parts are that the reserve runs under `HOLD_INTERRUPTS`, because the tracker can `elog(ERROR)` on its way to saying no and a longjmp out of Arrow's C++ frames is undefined behaviour, and that `pre_buffer` is switched off by name, because from Arrow 13 it defaults on and would allocate from I/O threads the tracker cannot see. All five pool sites use it — the three explicit ones and the two hidden defaults in `ReaderProperties` and `WriterP roperties::Builder`. Measured with `VmemTracker_GetMaxReservedVmemMB()`: a 2M-row write peaks at 252 MB against a ~20 MB backend baseline. **#1 encoding.** Done to the minimum you asked: both sides refuse a non-UTF8 `server_encoding` — the writer for column names as well, since Parquet's schema is UTF-8 — and the reader runs `pg_verifymbstr` on every string before it becomes a text. No conversion. **#5 positional vs field id.** Done, following tea's model: `ProjectionSet` names Iceberg field ids in output order (format ABI 2), the writer stamps `PARQUET:field_id` into the Parquet schema, the reader matches by id, a field the file lacks reads as a null-typed column, and an id-less column in a file can never be matched. The reader also accepts the promotions the spec allows — int32 as `bigint`, float as `double precision`. Name mapping for files without ids is #1989. **#8 INT96.** Done: `set_coerce_int96_timestamp_unit(MICRO)`. One caveat that turned out to be Arrow's, reproducible with pyarrow 21 and the same setting: pyarrow's deprecated INT96 writer stores a negative nanos-of-day for pre-1970 instants and Arrow's microsecond conversion reads those wrong; Spark, Hive and Impala files are fine, and coercing to nanoseconds instead would break every date outside 1677..2262. Noted in the code. Millisecond and nanosecond columns stay refused — that is the "real decision" you mentioned, and it is #1990. **#9 time, uuid.** Done. **#10 varchar(n) / char(n).** Resolved the other way round from the PR: `CREATE TABLE ... USING iceberg` now refuses `varchar(n)` and `char(n)` — and `numeric`, and anything else the format layer cannot store — through one function the DDL hook and the writer both call, so the two cannot drift. tea maps `text` only; Trino's Iceberg connector draws the same line. The read-side typmod coercion is gone with it. `LIKE` is refused too, because its columns are resolved after the hook has run. **#2 NUMERIC.** Left out on purpose, as you said was reasonable; your two traps — unconstrained `numeric` and precision above 38 — are recorded in #1988. Two things the same pass turned up beyond the review: dictionary-encoded columns, which Arrow restores whenever pandas wrote a categorical, used to pass the type check and decode their indexes as values — refused now; and `GetFormatRoutine` compared the format name case-sensitively while every other option value in the module does not. -- 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]
