leborchuk commented on PR #1951:
URL: https://github.com/apache/cloudberry/pull/1951#issuecomment-5571648987
Also here are some issues, I do not know should they fixed here or in a
future PR's. But I think they are quite important to be written. We could add
open issues to fix them later:
1. A failed write destroys a pre-existing file at that path.
arrow::io::FileOutputStream::Open(path) (parquet_write.cpp:390) truncates an
existing file — there's no O_EXCL — and parquet_discard
(parquet_write.cpp:95-102) then calls unlink(impl->path) unconditionally,
without knowing whether this writer created the file. So
datalake_parquet_write('/data/existing.parquet', 'select 1/0') truncates the
existing file at open, then deletes it on the error path. The same applies to
the resource-owner path (parquet_writer_release) and to abort(). Discarding a
file you created is right; discarding one you found is not. Since lake
data-file names are unique by construction, refusing an existing path is also
the semantics you want: open(2) with O_CREAT|O_EXCL and hand the fd to Arrow's
fd overload of FileOutputStream::Open, so the "did I create this" question is
answered by the kernel rather than assumed.
2. parquet_compression claims a check it doesn't make.
parquet_write.cpp:322-344 validates the name and reports "%s" is not a
compression this build can write (:338) — but nothing consults the build. An
Arrow packaged without ZSTD or GZIP support accepts 'zstd' here and fails at
the first row-group flush instead, long after rows have been accepted, with an
Arrow message rather than this one. arrow::util::Codec::IsAvailable(*out) is
available in Arrow 9 and makes the message true. Also std::string
requested(name) is compared case-sensitively, so 'SNAPPY' is rejected — worth a
pg_strcasecmp-equivalent since these arrive as SQL option strings.
3. A dropped column makes a table permanently unwritable.
DlArrowSchemaFromTupleDesc refuses attisdropped outright
(arrow_support.cpp:147-152). The comment justifies it as "nothing reads such a
file yet", but the consequence isn't about files — it's that ALTER TABLE ...
DROP COLUMN, which leaves a tombstone attribute in the TupleDesc forever, turns
every subsequent write into an error. Dropped columns aren't part of the
logical schema of new data, so skipping them (and shifting positions) is the
normal answer. If the intent is that the AM will pass a filtered descriptor,
that's worth saying here, because as written the format layer is the thing that
will refuse.
4. Arrow's allocations are invisible to Greenplum's memory accounting. Both
sides use arrow::default_memory_pool() (parquet_read.cpp:193,
parquet_write.cpp:362), and the writer deliberately holds up to a full row
group — 1Mi rows by default — before it can emit anything. That memory doesn't
go through palloc, so statement_mem and the vmem tracker don't see it, and a
segment can be pushed into OOM by a query that looks small to the resource
manager. I'm raising this as a question rather than a defect: itPR doesn't
state, and an arrow::MemoryPool subclass that reportsto the tracker is the
usual answer.
--
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]
