Skip to content

feat: writable external tables (INSERT/LOAD into stage files) - #24889

Merged
fengttt merged 29 commits into
matrixorigin:mainfrom
fengttt:feature/writable-external-table
Jun 15, 2026
Merged

feat: writable external tables (INSERT/LOAD into stage files)#24889
fengttt merged 29 commits into
matrixorigin:mainfrom
fengttt:feature/writable-external-table

Conversation

@fengttt

@fengttt fengttt commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What

Make external tables writable. An external table created with a new
write_file_pattern option accepts INSERT ... SELECT and LOAD, writing
CSV or JSONLine files into a stage:// destination. The pattern is a
strftime(3) template with two MO extensions:

  • %nNn random decimal digits
  • %U → a UUID

Each parallel write pipeline produces exactly one file. Reads, UPDATE/DELETE,
and read-only external tables are unchanged.

CREATE EXTERNAL TABLE t (a int, b varchar(20), c double)
INFILE{'filepath'='stage://s/part_*.csv', 'format'='csv',
       'write_file_pattern'='stage://s/part_%U.csv'}
FIELDS TERMINATED BY ',';

INSERT INTO t SELECT * FROM src;          -- writes one CSV file to the stage
LOAD DATA INFILE '...' INTO TABLE t ...;  -- same, from a source file

How

  • pkg/sql/colexec/externalwrite (new): strftime expander, ExternalWriter
    with a fileservice streaming sink (io.Pipe), CSV/JSONLine encoders. Encoders
    are const-vector aware and emit only the table's declared columns.
  • insert operator: third write mode ToExternal/insert_external alongside
    ToWriteS3/insert_table. Write config is carried on the Go InsertCtx
    built at compile time from TableDef.Createsql. The plan proto is unchanged; pipeline.Insert (proto) gained to_external + external_stmt_unix_nano so remote-run rebuilds the operator on the receiving CN.
  • compile: compileInsert routes external-write nodes to one writer op per
    source scope (no S3 merge/shuffle) → one file per pipeline, parallel across CNs.
  • planner: minimal external insert plan (build_insert/build_load);
    op-aware checkTableType allows writable-external for insert;
    initInsertStmt Pkey nil-guard (external tables have no PK); the modern DML
    binder defers external targets to the legacy planner.
  • DDL: validates write_file_pattern (must be stage://, csv/jsonline only,
    pattern must parse) and accepts it in the read-side option validators.

Semantics / limitations

  • Output formats: CSV and JSONLine only; destination must be stage://.
  • Empty pipeline → no file (lazy file creation).
  • Writes are not transactional (files are finalized at pipeline close); a
    failed statement may leave partial files. Documented as a v1 limitation.
  • UPDATE/DELETE on external tables remain unsupported.

Tests

  • Unit: pkg/sql/colexec/externalwrite (expander, encoders), build_ddl validator.
  • BVT: test/distributed/cases/stage/writable_external_table.{sql,result}
    CSV/JSONLine insert + readback, multi-file accumulation, LOAD into external,
    and all error cases. Passes 37/37 locally. Existing stage.sql (external reads
    • DML-rejection checks) still passes 100%.

Design + as-built notes: docs/design/writable_external_table_impl.md.

🤖 Generated with Claude Code

Make external tables writable when created with a WRITE_FILE_PATTERN option
(a strftime template with MO extensions %nN = n random digits and %U = UUID,
resolving to a stage:// path). Such tables accept INSERT ... SELECT and LOAD,
writing CSV or JSONLine files into the stage; each parallel pipeline writes one
file. Reads, UPDATE/DELETE, and read-only external tables are unchanged.

- pkg/sql/colexec/externalwrite: strftime expander, ExternalWriter with a
  fileservice streaming sink, CSV/JSONLine encoders (const-vector aware, emit
  only the declared columns).
- insert operator: third write mode ToExternal/insert_external alongside
  ToWriteS3/insert_table; write config carried on the Go InsertCtx (no proto
  change), built in compile from TableDef.Createsql.
- compile: compileInsert routes external-write nodes to one writer op per
  source scope (no S3 merge/shuffle).
- planner: minimal external insert plan (build_insert/build_load); op-aware
  checkTableType allows writable-external for insert; initInsertStmt Pkey guard;
  modern DML binder defers external targets to the legacy path.
- DDL: validate WRITE_FILE_PATTERN (stage:// + csv/jsonline + parseable) and
  accept it in the read-side option validators.
- docs/design + BVT case test/distributed/cases/stage/writable_external_table.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread pkg/sql/colexec/insert/insert.go
@fengttt

fengttt commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Review round 2 (at 2f3365c) — 10 inline findings posted, focused on the code added across the fix rounds and on test coverage. Several were verified empirically against the actual csvparser (constructed inputs through the real tokenizer), not just by reading.

Severity picture: three P1s are silent-corruption bugs in validator/writer gaps (multi-char terminator boundary overlap — (10,5) reads back as (1,5) with TERMINATED BY '00'; ENCLOSED BY '\' sneaking past the conflict check; jsonline records split by custom printable line terminators). Four P2s are round-trip losses the reader forces (trailing \r truncation even in quoted fields; \N-literal strings becoming NULL under non-default escapes and always under jsonline; jsonline coercing invalid UTF-8 to U+FFFD; enum('NULL') and whitespace-edged bit values). One P2 covers statement-scoped config drift (prepared EXECUTE freezes the pattern timestamp; remote CNs render TIMESTAMPs in a year-1 LMT zone due to the session-info codec). Plus %1N entropy and a consolidated test-coverage finding.

Adjacent issues found but not filed inline (pre-existing, beyond this PR's scope — listing for visibility):

  • ALTER TABLE <any external table> ADD/CHANGE/DROP COLUMN nil-derefs on tableDef.Pkey in buildAlterTable (external tables have no fake PK). Also means column-shape alters would bypass validateWriteFilePattern if the panic were fixed without a guard.
  • TRUNCATE TABLE silently no-ops on external tables with success status — misleading now that writable tables hold INSERTed data.
  • SHOW CREATE masks FILEPATH to '' for non-hive infile tables, so the emitted DDL recreates a write-only table; for stage:// paths the masking protects nothing. An empty stored JSONDATA is also accepted at CREATE but rejected at read.
  • Maintainability: CSV-option defaulting now lives in three hand-synced copies (validator / buildExternalInsertArg / NewExternalWriter) — worth a single ResolveCSVOptions(tail); the REPLACE sentinel is matched by full message string across packages (a structural marker would be safer); WriterConfig.Header/writeCSVHeader/expandedPath/writer-side rowsWritten are dead or write-only; FileServiceWriter.Abort's Group.Wait can block teardown indefinitely on a hung backend (detaching the wait would bound it).

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked the latest head. The CSV option round-trip work looks much better, but one substantive issue still remains.

SHOW CREATE TABLE still drops the external table's read FILEPATH, so writable external tables are not actually recreatable from their own DDL.

formatInfileExternalOptionsForShowCreate() still hardcodes filepath := "" for non-Hive external tables and only preserves param.Filepath when HivePartitioning is true (pkg/sql/plan/build_show_util.go:960-975). The new BVT output still shows this exact problem on the writable-table cases, e.g. ext_csv and t_all both emit INFILE{'FILEPATH'='', ... 'WRITE_FILE_PATTERN'=...} (test/distributed/cases/stage/writable_external_table.result:13-15, test/distributed/cases/stage/writable_external_csv_options.result:260-262).

That means the shown DDL does not recreate the original table semantics: it preserves the write pattern, but loses the read glob. For writable external tables this is not just cosmetic, because SHOW CREATE TABLE is supposed to be re-executable and MatrixOne's snapshot/PITR/restore paths fetch table DDL via show create table (pkg/frontend/snapshot_restore_with_ts.go:252-268, pkg/frontend/pitr.go:1583-1592, pkg/frontend/snapshot.go:2072-2080). Replaying the downgraded DDL can silently turn a readable writable-external table into one with no read path at all.

I think this still needs to be fixed before approval: preserve the external table's FILEPATH in SHOW CREATE TABLE / restoreDDL for these writable tables, not only the WRITE_FILE_PATTERN.

fengttt and others added 3 commits June 12, 2026 16:53
…overage)

Corruption fixes (several verified against the real csvparser):
- needsEnclosure also encloses a value whose suffix is a proper prefix of a
  multi-char field terminator: the reader matches the terminator across the
  value boundary, so (10,5) with TERMINATED BY '00' read back as (1,5).
- The escape==enclosure conflict is rejected for the DEFAULT backslash escape
  too (ENCLOSED BY '\' previously validated and double-unescaped on read),
  and control characters cannot be the escape.
- jsonline only accepts LINES TERMINATED BY '\n'/'\r\n': JSON strings have no
  enclosure, so a printable terminator inside a value split records.
- addEscape rewrites CR as E+'r': the reader strips one trailing CR per
  record even from enclosed fields, so values ending in CR were truncated.
  With ESCAPED BY '' a trailing CR in the last column is rejected instead.
- Values no encoding can round-trip now error instead of corrupting: a string
  of exactly \N under a non-default/disabled escape (the reader null-matches
  enclosed fields outside the backslash flavor) and always under jsonline
  (JsonNull compare post-decode); invalid UTF-8 strings under jsonline (the
  reader rewrites such bytes to U+FFFD).
- enum labels are enclosed (a bare NULL token read back as SQL NULL); the
  reader no longer TrimSpaces bit fields (raw bytes: 'A ' lost its space,
  whitespace-only values became NULL) — bit 32/13/10 now round-trip.
- %nN needs n >= 6 to qualify as the uniqueness directive (%1N has 10
  outcomes; parallel writers collided by pigeonhole).

Statement-scoped config drift:
- The writer timestamp is re-resolved from defines.StartTS at Prepare and at
  remote-run encode: prepared EXECUTEs reuse the cached Compile, so the
  config value froze at the first execution and a daily EXECUTE kept writing
  into day-1's %Y%m%d directory.
- The session time zone travels to remote CNs via new pipeline.Insert fields
  (external_tz_name + offset-at-statement fallback): the generic session
  codec round-trips zones as a year-1 LMT fixed offset, shifting rendered
  TIMESTAMPs by minutes on remote pipelines.
- TRUNCATE on a writable external table errors instead of reporting success
  while the stage files survive.

Coverage:
- Unit tests: insert operator external path with a mock writer (NOT NULL
  matrix incl. const-null and the NotNull-only flag; Reset/Free must Abort,
  never Close), FileServiceWriter Close-persists/Abort-discards/idempotency
  on a real LocalETLFS, remote-run tail-config parity (every FIELDS/LINES
  field local==decoded, incl. NoEscape; tz name), appendJSONFloat pinned
  byte-identical to encoding/json across regimes plus NaN/Inf, round-trip
  guard errors, needsEnclosure boundary cases, %nN entropy cases.
- BVT: UPDATE/DELETE/TRUNCATE rejections, terminator-boundary table, enum
  'NULL' label round-trip, trailing-CR round-trip, \N rejection (csv custom
  escape + jsonline), bit whitespace bytes, and DDL error cases for
  ENCLOSED BY '\', jsonline custom terminator, and %2N. 140/140 + 123/123
  against a freshly built 2-CN cluster; key fixes verified live first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The all-options csv case used FIELDS TERMINATED BY ';'. CI's mo-tester
statement splitter breaks on a semicolon inside a quoted SQL string, which
desynchronized the script from its expected results (1 direct failure) and
skipped the file's cleanup, leaking the wcsv database into later cases'
`show databases` output (the temporary_table_limitation and tenant/cache
failures in the same run). The local mo-tester handles quoted semicolons,
which is why the file passed locally.

Use '^' as the separator instead; the case still exercises a custom
single-byte separator combined with quote/escape/prefix/terminator options.
Results regenerated; both stage BVT files pass locally (123/123 + 140/140).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The INFILE show-create form masked FILEPATH to '' for non-hive external
tables and always emitted the optional keys even when empty. For writable
external tables that made the emitted DDL semantically lossy: snapshot/PITR
restore replays DDL fetched via SHOW CREATE, so a restored table kept its
WRITE_FILE_PATTERN but lost its read glob — writable but unreadable — and an
embedded 'JSONDATA'='' was accepted at CREATE only to fail at read time.

Writable tables (WRITE_FILE_PATTERN present) now emit the real FILEPATH and
omit empty optional keys; read-only external tables keep the legacy masked
output byte-for-byte. Verified live: SHOW CREATE -> drop -> execute the
emitted DDL verbatim -> the recreated table reads the pre-existing stage
files and accepts new writes, for both csv and jsonline. The BVT now
recreates ext_csv and ext_jl from their emitted DDL and asserts the combined
counts (3 -> 6 -> 9); unit tests pin the writable form (FILEPATH present, no
empty keys) and the unchanged read-only form. 149/149 + 123/123 locally.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengttt

fengttt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@XuPeng-SH Fixed in 3ab7a82 — you're right that with restore replaying SHOW CREATE output this stopped being cosmetic.

For external tables with a WRITE_FILE_PATTERN, formatInfileExternalOptionsForShowCreate now emits the real FILEPATH and omits empty optional keys (the second half mattered too: an embedded 'JSONDATA'='' was accepted at CREATE but rejected by the read-side validator, so even with FILEPATH restored the recreated table couldn't read). Read-only external tables keep the legacy masked output byte-for-byte, so no existing expectations outside this PR change.

Verified live on the cluster as a full loop: SHOW CREATE → drop → execute the emitted DDL verbatim → the recreated table reads the pre-existing stage files and accepts new writes — for both csv and jsonline (which emits its real JSONDATA='object'). The BVT now does the same: it recreates ext_csv and ext_jl from their emitted DDL mid-test and asserts the combined counts (3 → 6 → 9 across the recreation boundary), and unit tests pin both the writable form (FILEPATH present, no empty keys) and the unchanged read-only form.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few remaining unhappy-path/correctness blockers in the current head.

  1. Compressed writable tables can be created, but the write path always emits plain bytes. validateWriteFilePattern accepts COMPRESSION='gzip' and compressed write-path suffixes, while buildExternalInsertArg/WriterConfig do not carry compression and externalWriter streams raw CSV/JSONLine. The read path later calls crt.GetUnCompressReader(..., param.Extern.CompressType, param.Fileparam.Filepath, ...), and crt.GetCompressType auto-detects suffixes like .gz/.bz2/.lz4 when compression is empty/auto. So a table can be created with a gzip compression setting or a .gz write pattern, INSERT can succeed, and then SELECT tries to decompress raw CSV/JSONLine and fails. Please either implement compression on the write path or reject writable external tables whose effective compression for produced files is not none.

  2. CSV comment handling can corrupt valid writer output. External CSV readers always set Comment: '#', and csvparser.readRecord skips any row whose recordBuffer[0] == '#' after quotes are stripped but before unescape. The writer emits "#lost",1\n for a first-column string value '#lost', so the same table can silently drop the inserted row on readback. There is also a panic edge case here: if all fields are empty strings, e.g. ""\n or "",""\n, recordBuffer is empty and the comment check still indexes recordBuffer[0]. Please either disable comment processing for these external table reads, or make the writer/validator protect/reject the affected values; coverage should include leading # and all-empty string rows.

  3. Writable DDL accepts CSV delimiter configs that the reader refuses at open time. For example, FIELDS TERMINATED BY '#' passes the new writable validation, but the actual CSV parser rejects it because the reader's comment marker is also #. Delimiters starting with invalid parser bytes such as newline, carriage return, or quote are the same class of issue. This means the table can be created and written, but any read fails before parsing. Writable DDL should mirror the parser delimiter validation or construct/check the parser config during validation.

  4. Duplicate external options can make validation check a different configuration than the one later stored/executed. validateWriteFilePattern uses getRawOption, which returns the first format/jsondata, but the option initializers later walk the whole slice and later duplicates win. A table can therefore validate as format='csv' and then be stored/executed as format='parquet', causing runtime external write format "parquet" after CREATE succeeded. Duplicate jsondata can similarly validate as object but read as array. Please reject duplicate option keys relevant to writable external tables before validation/normalization.

fengttt and others added 2 commits June 12, 2026 21:19
…+ '#' combined

The previous CI run still failed on writable_external_csv_options at the t_all
"everything at once" statement, cascading (via a skipped DROP DATABASE that
leaked the wcsv schema) into temporary_table_limitation and tenant/cache
`show databases` mismatches — the same shape as the earlier quoted-semicolon
failure.

Root cause: t_all's create line was the only one combining an escaped-quote
enclosure ('\'') with later option tokens AND a '#'-bearing terminator
('#EOL#') on one line. Each token is fine alone in CI's mo-tester — t_squote
uses enclosed by '\'' (immediately followed by ';') and passes, and the main
file's `lines terminated by '#'` passes — but the combination on one long
line desynced CI's statement splitter from the .result file.

Swap t_all to a plain '@' enclosure and a 'qEOLq' terminator (no escaped
quote, no '#'); the case still combines a custom separator, enclosure,
escape, STARTING BY and a multi-char line terminator, and single-quote
enclosure stays covered by t_squote. Result regenerated; the data still
round-trips all 11 tricky rows. Passes 123/123 locally.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng-SH)

1. Compression: validateWriteFilePattern rejects any effective compression —
   an explicit COMPRESSION option, or a .gz/.bz2/.lz4 suffix auto-detected
   from the read FILEPATH glob or the WRITE_FILE_PATTERN. The writer streams
   plain bytes, so a compressed config produced files the read path would try
   to decompress and fail. (effectiveWriteCompression inlines crt's decision
   to avoid the plan<-crt import cycle.)

2. CSV '#' comment marker: the external CSV reader hardcoded Comment='#',
   which silently dropped any row whose first column started with '#' (after
   quote-stripping, so enclosure could not protect it) and could index
   recordBuffer[0] out of range on an all-empty-string row. Disable the
   comment marker for writable-table reads (a value starting with '#' is data;
   '#' also becomes a usable field terminator), keep it for read-only tables,
   and guard the parser's comment skip with len(recordBuffer) > 0.

3. Delimiter validation: reject a FIELDS TERMINATED BY whose first byte is a
   quote, CR, LF or NUL — the CSV reader rejects those at open, so such a
   table could be created and written but never read.

4. Duplicate option keys: reject duplicate format/jsondata/compression/
   filepath/write_file_pattern before validation. getRawOption returns the
   first, the read-side init keeps the last, so a table could validate as csv
   and execute/read as parquet.

Tests: validateWriteFilePattern cases for compression (option + read/write
suffix), duplicate keys, and field-terminator bytes; csvparser tests for the
empty-record no-panic and comment-disabled paths; BVT round-trips a
'#'-leading value, an all-empty row and a '#' separator, plus DDL error
cases for compression, duplicate keys and a quote terminator. 160/160 +
123/123 on the 2-CN cluster; all four fixes verified live.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengttt

fengttt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@XuPeng-SH Thanks — all four were real. Fixed in 71595e6, each verified live on the 2-CN cluster.

1. Compression. validateWriteFilePattern now rejects any effective compression: an explicit COMPRESSION option, or a .gz/.bz2/.lz4-class suffix auto-detected from either the read FILEPATH glob or the WRITE_FILE_PATTERN (mirroring crt.GetCompressType's decision, inlined to avoid a plan←crt import cycle). So COMPRESSION='gzip' and a *.csv.gz read glob and a %U.csv.gz write pattern are all rejected at CREATE.

2. CSV # comment. The reader hardcoded Comment: '#', dropping any first-column value starting with # (after quote-stripping, so enclosure couldn't save it) and risking an out-of-range recordBuffer[0] on an all-empty row. I disabled the comment marker for writable-table reads (detected via GetWriteFilePattern on the read param) — a #-leading value is now data and # becomes a usable separator — while read-only external tables keep # for backward compatibility. The parser's comment skip is also now guarded with len(recordBuffer) > 0 (fixes the panic for all readers). BVT round-trips #lead, an all-empty row, and a # separator.

3. Delimiter validation. A FIELDS TERMINATED BY whose first byte is a quote, CR, LF or NUL is rejected at DDL (mirroring the reader's validDelim/NewCSVParser). With the comment marker disabled for writable reads, # is now a legal field terminator and round-trips.

4. Duplicate option keys. validateWriteFilePattern now rejects duplicate format/jsondata/compression/filepath/write_file_pattern up front (reusing rejectDuplicateKeys), so validation and the last-wins read-side init can't disagree — no more validate-as-csv / run-as-parquet.

Unit tests cover the compression (option + read-suffix + write-suffix), duplicate-key, and terminator-byte cases, plus csvparser tests for the empty-record no-panic and comment-disabled paths. BVT: 160/160 + 123/123 locally.

@fengttt

fengttt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

@XuPeng-SH Thanks — all four were real. Fixed in 71595e6, each verified live on the 2-CN cluster.

1. Compression. validateWriteFilePattern now rejects any effective compression: an explicit COMPRESSION option, or a .gz/.bz2/.lz4-class suffix auto-detected from either the read FILEPATH glob or the WRITE_FILE_PATTERN (mirroring crt.GetCompressType's decision, inlined to avoid a plan←crt import cycle). So COMPRESSION='gzip' and a *.csv.gz read glob and a %U.csv.gz write pattern are all rejected at CREATE.

2. CSV # comment. The reader hardcoded Comment: '#', dropping any first-column value starting with # (after quote-stripping, so enclosure couldn't save it) and risking an out-of-range recordBuffer[0] on an all-empty row. I disabled the comment marker for writable-table reads (detected via GetWriteFilePattern on the read param) — a #-leading value is now data and # becomes a usable separator — while read-only external tables keep # for backward compatibility. The parser's comment skip is also now guarded with len(recordBuffer) > 0 (fixes the panic for all readers). BVT round-trips #lead, an all-empty row, and a # separator.

3. Delimiter validation. A FIELDS TERMINATED BY whose first byte is a quote, CR, LF or NUL is rejected at DDL (mirroring the reader's validDelim/NewCSVParser). With the comment marker disabled for writable reads, # is now a legal field terminator and round-trips.

4. Duplicate option keys. validateWriteFilePattern now rejects duplicate format/jsondata/compression/filepath/write_file_pattern up front (reusing rejectDuplicateKeys), so validation and the last-wins read-side init can't disagree — no more validate-as-csv / run-as-parquet.

Unit tests cover the compression (option + read-suffix + write-suffix), duplicate-key, and terminator-byte cases, plus csvparser tests for the empty-record no-panic and comment-disabled paths. BVT: 160/160 + 123/123 locally.

2 is the wrong fix. This is a parser bug (old code) and should be fixed in the read path. Vibing it now.

Supersedes the earlier per-writable-table comment workaround. The CSV
parser's comment handling had two problems: cfg.Comment was accepted but
never wired into the parser (dead code), and where it would have applied the
check was on recordBuffer[0] AFTER unquoting, so an enclosed value like
"#x" would have been dropped as a comment.

Fix the parser instead of special-casing writable tables:
- CSVConfig.Comment is now a string (was byte); the empty string (default)
  means no comment marker, so every line is data. A configured marker is
  matched against the line's RAW prefix before unquoting, so an enclosed
  "#x" is data and only an unquoted line beginning with the marker is
  skipped. Consecutive comment lines are skipped; the check is gated to a
  fresh line start so it composes with LINES STARTING BY.
- Wire cfg.Comment into the parser so the option actually takes effect.
- Both CSV callers (external read, LOAD DATA) now default the marker empty.
  This matches MySQL LOAD, which does not treat '#' lines as comments, and
  is behavior-preserving: the marker was never actually applied before.

Because the marker no longer shares the tokenizer byte stop-sets, a '#'
field terminator no longer collides with it, so the DDL no longer rejects
'#' as a writable field terminator.

Tests: csvparser cases for the empty-marker (all data), a configured marker
matched on the raw prefix (quoted "#x" is data, unquoted '#' line skipped),
and the all-empty-row no-panic; the writable field-terminator test allows
'#'. BVT round-trips a '#'-leading value and an all-empty row. Verified no
regression: load_data 871/871 and the full stage dir 1411/1411 pass, plus
the two writable files 160/160 + 123/123.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengttt

fengttt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

Correction on the CSV comment-marker fix (#2), now in d8b3806 — my earlier "disable comment for writable reads" was the wrong approach. Fixed the parser properly instead:

  • Digging in, CSVConfig.Comment was accepted but never wired into the parser (dead code), and the check it would have done was on recordBuffer[0] after unquoting — which is exactly why an enclosed "#x" would have been dropped.
  • CSVConfig.Comment is now a string, defaulting to empty = no comment marker, so every line is data. Both CSV callers (external read and LOAD DATA) default it empty — matching MySQL LOAD, which doesn't treat # lines as comments. This is behavior-preserving since the marker was never actually applied before.
  • When a marker is configured, it's matched against the line's raw prefix before unquoting, so an enclosed "#x" is data and only an unquoted line beginning with the marker is skipped (composes with LINES STARTING BY, skips consecutive comment lines).
  • Since the marker no longer shares the tokenizer byte stop-sets, # is a valid field terminator again (DDL no longer rejects it).

Verified no regression from the global default change: load_data 871/871 and the full stage dir 1411/1411 pass, plus the two writable BVT files (160/160 + 123/123). New csvparser unit tests cover the empty-marker, raw-prefix-match, and all-empty-row-no-panic cases.

…option

Builds on the configurable-comment parser change: external tables now accept
a 'comment' option (INFILE{... 'comment'='#'} / 'comment'='REM'), plumbed via
plan.GetCSVComment into the CSV reader's CSVConfig.Comment. Absent/empty (the
default) means no comment marker — every line is data. A configured marker
skips lines whose RAW prefix matches it (so an enclosed "#x" or a mid-line '#'
stays data). The option is added to the allowed external-option keys and the
three Init*Param validators.

BVT load_data/external_csv_comment.sql reads one fixture three ways — default
(6 rows, all data), comment='#' (5 rows; the '#c1,c2' line skipped while
"#quoted" and 3,#midhash remain), comment='REM' (5 rows; 'REMx,REMy' skipped).
Verified live; 19/19. plan/external/csvparser unit suites pass, static-check
clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 42 out of 43 changed files in this pull request and generated 7 comments.

Files not reviewed (1)
  • pkg/pb/pipeline/pipeline.pb.go: Generated file

Comment thread pkg/sql/util/csvparser/csv_parser.go
Comment thread pkg/sql/util/csvparser/csv_parser.go
Comment thread pkg/sql/util/csvparser/csv_parser_test.go Outdated
Comment thread pkg/sql/plan/build_ddl.go
Comment thread pkg/sql/plan/build_show_util_test.go Outdated
Comment thread pkg/sql/colexec/externalwrite/encode.go Outdated
Comment thread pkg/sql/plan/build_show_util.go Outdated
fengttt and others added 3 commits June 12, 2026 22:59
…alloc)

- Reword four doc/test comments that rendered a curly quote where they meant
  to describe SQL quote-doubling / empty ESCAPED BY. gofmt's doc-comment
  formatter rewrites a literal '' into a curly quote, so the comments now use
  words ("an empty FIELDS ESCAPED BY", "a single quote written as two single
  quotes") instead of the bare '' sequence.
- csvparser: the NewCSVParser validation now only checks the field delimiter,
  so its error message drops the misleading "or comment delimiter" wording
  (and the one test matching the exact string is updated).
- csvparser: the per-line comment-marker check no longer allocates — the
  parser holds the marker as []byte (converted once at construction) and
  compares with bytes.Equal instead of string(bs).
- Fix the stale "Comment == 0" test comment now that Comment is a string.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CI failure was 3 cases in table/external_table.sql, all consequences of
the directed comment-marker change (default empty, validation checks only the
field delimiter):

- ex_table_7 uses FIELDS TERMINATED BY '#'. With no comment marker by default,
  '#' no longer collides with one, so the table now reads its file instead of
  erroring. Its result is switched to the @Separator:table (boxed) format
  because col20 contains an internal run of spaces that the plain space-
  separated .result format cannot represent unambiguously.
- ex_table_8 / ex_table_9 (FIELDS TERMINATED BY '\n' / '\r') still error, now
  with the reworded "invalid field delimiter" message.

external_table.sql passes 249/249 locally. (The 'or comment delimiter' string
appeared in no other test.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked the latest head (66526fa351aeaf4dc149f229ea3ced7b1f15237b). I think a few correctness blockers still need changes before this can land.

  1. Writable CSV external tables with a configured comment marker can silently drop rows written by this same feature. The reader now passes plan.GetCSVComment(extern) into CSVConfig (pkg/sql/colexec/external/types.go:291), and the CSV parser skips a record when the raw line prefix matches the comment marker before LINES STARTING BY is stripped (pkg/sql/util/csvparser/csv_parser.go:568). However, the writer config rebuilt by buildExternalInsertArg does not carry this marker (pkg/sql/compile/operator.go:989, pkg/sql/colexec/externalwrite/writer.go:50), and validateWriteFilePattern does not reject it. For example, with comment='1', inserting a row whose first field serializes as 1 writes 1,...\n, and a later read treats it as a comment. With LINES STARTING BY 'R>' and comment='R'/comment='R>', every writer-produced row can be skipped. Please either reject non-empty COMMENT for writable external tables or make the writer guarantee that the raw record prefix cannot match the configured comment marker.

  2. SHOW CREATE still drops the external CSV comment option. formatInfileExternalOptionsForShowCreate emits filepath/compression/format/jsondata/write_file_pattern for writable infile tables (pkg/sql/plan/build_show_util.go:961), and formatS3ExternalOptionsForShowCreate has the same gap for S3 options (pkg/sql/plan/build_show_util.go:1000). Since reader semantics now depend on comment (pkg/sql/plan/utils.go:2054, pkg/sql/colexec/external/types.go:291), replaying SHOW CREATE or restore DDL changes what rows are returned.

  3. Generated columns are accepted on writable external tables but are not protected or recomputed in the legacy external INSERT/LOAD path. The writable-table validator rejects auto_increment but not GeneratedCol (pkg/sql/plan/build_ddl.go:6038). The external INSERT path uses the minimal legacy builder (pkg/sql/plan/build_insert.go:265) where getInsertColsFromStmt includes generated columns by default (pkg/sql/plan/build_insert.go:375), bypassing the normal binder logic that rejects explicit generated columns and recomputes generated expressions (pkg/sql/plan/bind_insert.go:1108, pkg/sql/plan/bind_insert.go:1435). LOAD into writable external tables similarly uses the legacy project-building path (pkg/sql/plan/build_load.go:521) instead of the modern generated-column filtering in bind_load.go (pkg/sql/plan/bind_load.go:85). This can allow arbitrary values to be stored for generated columns, or store NULL/defaults when the generated column is omitted. Please reject generated columns for writable external tables or route this path through the generated-column rewrite.

Also, the current PR checks show Matrixone Utils CI / Coverage failing.

Local verification note: I tried focused Go test commands for the touched CSV parser/external writer packages, but this checkout fails during build on github.com/unum-cloud/usearch/golang cgo symbols before the tests run, so I could not get a clean local test signal.

fengttt and others added 2 commits June 13, 2026 10:27
… and compression helper

- TestNewCSVParserCommentOption: external COMMENT option flows into the CSV
  reader (default empty => every line data; '#'/'REM' skip matching raw-prefix
  lines; enclosed value starting with marker stays data)
- TestGetCSVComment + extended TestInitInfileParam_Plain: comment and
  write_file_pattern option parsing
- TestEffectiveWriteCompression: compression inference helper

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ject generated cols

Addresses three review findings on writable external tables:

1. CSV comment-marker round-trip (writer-side fix, not a DDL rejection):
   the reader skips a line whose raw prefix matches the COMMENT marker, so a
   writer row whose first field serialized to that prefix (e.g. comment='1',
   first field 1) was silently dropped on readback. The writer now encloses the
   first field of a row ONLY when its unenclosed line prefix would collide with
   the marker (firstFieldStartsComment), so the line begins with the enclosure
   byte and reads back as data. String-like columns are already always enclosed;
   the guard matters for unenclosed types. COMMENT flows into WriterConfig.

2. SHOW CREATE now round-trips the COMMENT option (writable INFILE, read-only
   INFILE, and S3 branches); reader semantics depend on it, so restore/replay
   DDL must preserve it. Omitted when unset.

3. Generated columns are now rejected for writable external tables: the minimal
   external INSERT/LOAD plan neither filters explicit writes nor recomputes the
   generated expression, so values would be stored arbitrary or NULL/default.

Tests: writer comment-guard unit test with a real csvparser round-trip
(collision enclosed, non-collision left bare, no-marker regression); validator
accepts COMMENT and rejects generated columns; SHOW CREATE comment round-trip
for INFILE+S3; BVT ext_bad19 (generated column rejected).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fengttt

fengttt commented Jun 13, 2026

Copy link
Copy Markdown
Contributor Author

I found a few remaining unhappy-path/correctness blockers in the current head.

  1. Compressed writable tables can be created, but the write path always emits plain bytes. validateWriteFilePattern accepts COMPRESSION='gzip' and compressed write-path suffixes, while buildExternalInsertArg/WriterConfig do not carry compression and externalWriter streams raw CSV/JSONLine. The read path later calls crt.GetUnCompressReader(..., param.Extern.CompressType, param.Fileparam.Filepath, ...), and crt.GetCompressType auto-detects suffixes like .gz/.bz2/.lz4 when compression is empty/auto. So a table can be created with a gzip compression setting or a .gz write pattern, INSERT can succeed, and then SELECT tries to decompress raw CSV/JSONLine and fails. Please either implement compression on the write path or reject writable external tables whose effective compression for produced files is not none.
  2. CSV comment handling can corrupt valid writer output. External CSV readers always set Comment: '#', and csvparser.readRecord skips any row whose recordBuffer[0] == '#' after quotes are stripped but before unescape. The writer emits "#lost",1\n for a first-column string value '#lost', so the same table can silently drop the inserted row on readback. There is also a panic edge case here: if all fields are empty strings, e.g. ""\n or "",""\n, recordBuffer is empty and the comment check still indexes recordBuffer[0]. Please either disable comment processing for these external table reads, or make the writer/validator protect/reject the affected values; coverage should include leading # and all-empty string rows.
  3. Writable DDL accepts CSV delimiter configs that the reader refuses at open time. For example, FIELDS TERMINATED BY '#' passes the new writable validation, but the actual CSV parser rejects it because the reader's comment marker is also #. Delimiters starting with invalid parser bytes such as newline, carriage return, or quote are the same class of issue. This means the table can be created and written, but any read fails before parsing. Writable DDL should mirror the parser delimiter validation or construct/check the parser config during validation.
  4. Duplicate external options can make validation check a different configuration than the one later stored/executed. validateWriteFilePattern uses getRawOption, which returns the first format/jsondata, but the option initializers later walk the whole slice and later duplicates win. A table can therefore validate as format='csv' and then be stored/executed as format='parquet', causing runtime external write format "parquet" after CREATE succeeded. Duplicate jsondata can similarly validate as object but read as array. Please reject duplicate option keys relevant to writable external tables before validation/normalization.

All fixed and resolved.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked the latest head (c604e3919093652626d9d09a83bc2b4fa0675c82). The previous generated-column rejection and COMMENT/SHOW-CREATE plumbing are moving in the right direction, but I still see correctness blockers.

  1. COMMENT is still unsafe for writable tables; several accepted configurations make the table skip rows it just wrote. The reader applies the comment marker to the raw line prefix before LINES STARTING BY is consumed (pkg/sql/util/csvparser/csv_parser.go:568), and newCSVParserFromReader passes that marker for all formats, including jsonline (pkg/sql/colexec/external/types.go:291). The jsonline writer always starts a record with LineStartingBy + { and has no comment guard (pkg/sql/colexec/externalwrite/encode.go:315), while DDL still allows COMMENT on writable jsonline tables (pkg/sql/plan/build_ddl.go:5998). So format='jsonline' + COMMENT='{' drops every inserted row on readback.

    The CSV-side guard is also incomplete. It only runs for non-NULL, currently-unquoted first fields (pkg/sql/colexec/externalwrite/encode.go:59, pkg/sql/colexec/externalwrite/encode.go:96), and firstFieldStartsComment intentionally returns false when the marker is contained in the fixed LINES STARTING BY prefix (pkg/sql/colexec/externalwrite/encode.go:153). Examples that still corrupt data: COMMENT='REM' LINES STARTING BY 'REM:' skips every writer row; COMMENT='"' skips rows whose first column is always quoted (varchar, enum, bit, json, or numeric/date values that need enclosure); and COMMENT='\N'/COMMENT='\' skips rows whose first column is NULL because the NULL sentinel is written before the guard. Please reject unprotectable COMMENT configurations at DDL time (or reject COMMENT for writable jsonline) and make the CSV writer error/guard for NULL and already-quoted first-field collisions.

  2. A failed parallel statement can still leave successfully closed sibling files visible. insert_external finalizes each operator's file as soon as that pipeline sees end-of-input and then nils the writer (pkg/sql/colexec/insert/insert.go:265); Reset/Free only abort a writer that is still non-nil (pkg/sql/colexec/insert/types.go:127, pkg/sql/colexec/insert/types.go:159), and externalWriter.Close finalizes the file (pkg/sql/colexec/externalwrite/writer.go:208, pkg/fileservice/file_service_writer.go:86). In a multi-pipeline INSERT/LOAD, one scope can finish and close a complete output file before a sibling scope later hits an expression/read/write error. The statement returns failure, but the already-closed file remains in the stage; retrying the statement then duplicates that scope's rows. The earlier abort-on-live-writer fix covers partial live streams, but not completed sibling streams in the same failed statement. This needs a statement-level commit/abort strategy, or a way to keep produced files temporary until all parallel writers have succeeded.

  3. SHOW CREATE still does not faithfully round-trip LINES TERMINATED BY '\r\n'. The formatter maps both stored "\n" and stored "\r\n" to LINES TERMINATED BY '\\n' (pkg/sql/plan/build_show_util.go:626), while the read/write configs do preserve and honor CRLF (pkg/sql/colexec/external/types.go:277, pkg/sql/compile/operator.go:1023, pkg/sql/colexec/externalwrite/encode.go:333). Replaying SHOW CREATE changes the external-table format from CRLF to LF, so writable tables are still not exact-format recreatable for this supported option.

Also, the latest checks still show Matrixone Utils CI / Coverage failing.

Local verification note: I tried focused tests for csvparser, externalwrite, and plan, but this checkout still fails during build on github.com/unum-cloud/usearch/golang cgo symbols before tests run, so I could not get a local assertion signal.

fengttt and others added 3 commits June 13, 2026 23:42
The CSV reader matches the COMMENT marker against the raw line prefix
before LINES STARTING BY is consumed, and the marker applies to all
formats (jsonline included). Every jsonline record the writer produces
deterministically begins with LINES STARTING BY + '{', and JSON has no
enclosure mechanism to hide it the way the CSV writer's first-field guard
does. So a writable jsonline table with COMMENT='{' would skip every row
it just wrote on readback.

CSV keeps COMMENT support (the firstFieldStartsComment encloser makes it
round-trip); jsonline now rejects any non-empty COMMENT at DDL time.

Unit tests + a BVT error case (ext_bad20) added.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CSV writer's first-field comment guard only ran for non-NULL,
unenclosed first fields and intentionally gave up when the marker was
contained in the LINES STARTING BY prefix, so several configs still made
the reader skip rows the writer produced (the marker is matched on the
raw line prefix before unquoting / before STARTING BY is consumed):

  - COMMENT='REM' + LINES STARTING BY 'REM:'  -> every row skipped
  - COMMENT='"' (enclosure byte)              -> all enclosed first fields
  - COMMENT='\' (escape byte)                 -> escaped first fields
  - COMMENT=',' (field terminator)            -> empty first field rows
  - COMMENT='\N' (NULL sentinel)              -> NULL first column rows

These cannot be fixed by the writer's enclose-the-field guard (the
collision IS the enclosure/escape/terminator/sentinel byte), so reject
them at DDL time in a new validateWritableComment:

  - COMMENT and LINES STARTING BY are mutually exclusive for CSV
  - COMMENT's first byte must not be the enclosure, escape, or field
    terminator byte
  - COMMENT must not collide with the NULL sentinel \N

The writer guard (firstFieldStartsComment) drops its buggy
len(marker) <= len(startingBy) exemption, now moot since COMMENT and
LINES STARTING BY can no longer coexist.

Verified the reader's LINES STARTING BY handling is correct (the
terminator is preserved across the put-back in readRecord).

Unit tests cover every new rejection plus the still-valid cases; BVT
adds ext_bad21..25 error cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The formatter mapped both stored "\n" and "\r\n" to LINES TERMINATED BY
'\n', so replaying SHOW CREATE silently downgraded a CRLF writable
external table to LF — even though the read/write configs preserve and
honor CRLF (external/types.go, operator.go, externalwrite/encode.go).

Render \r\n as its own escape sequence, keeping the existing
doubled-backslash convention (the result is delivered through a
double-quoted SELECT literal that consumes one backslash level). Extract
the rendering into a small formatLinesTerminatedBy helper with a unit
test pinning \n and \r\n distinct.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fengttt

fengttt commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

I rechecked the latest head (c604e3919093652626d9d09a83bc2b4fa0675c82). The previous generated-column rejection and COMMENT/SHOW-CREATE plumbing are moving in the right direction, but I still see correctness blockers.

  1. COMMENT is still unsafe for writable tables; several accepted configurations make the table skip rows it just wrote. The reader applies the comment marker to the raw line prefix before LINES STARTING BY is consumed (pkg/sql/util/csvparser/csv_parser.go:568), and newCSVParserFromReader passes that marker for all formats, including jsonline (pkg/sql/colexec/external/types.go:291). The jsonline writer always starts a record with LineStartingBy + { and has no comment guard (pkg/sql/colexec/externalwrite/encode.go:315), while DDL still allows COMMENT on writable jsonline tables (pkg/sql/plan/build_ddl.go:5998). So format='jsonline' + COMMENT='{' drops every inserted row on readback.
    The CSV-side guard is also incomplete. It only runs for non-NULL, currently-unquoted first fields (pkg/sql/colexec/externalwrite/encode.go:59, pkg/sql/colexec/externalwrite/encode.go:96), and firstFieldStartsComment intentionally returns false when the marker is contained in the fixed LINES STARTING BY prefix (pkg/sql/colexec/externalwrite/encode.go:153). Examples that still corrupt data: COMMENT='REM' LINES STARTING BY 'REM:' skips every writer row; COMMENT='"' skips rows whose first column is always quoted (varchar, enum, bit, json, or numeric/date values that need enclosure); and COMMENT='\N'/COMMENT='\' skips rows whose first column is NULL because the NULL sentinel is written before the guard. Please reject unprotectable COMMENT configurations at DDL time (or reject COMMENT for writable jsonline) and make the CSV writer error/guard for NULL and already-quoted first-field collisions.
  2. A failed parallel statement can still leave successfully closed sibling files visible. insert_external finalizes each operator's file as soon as that pipeline sees end-of-input and then nils the writer (pkg/sql/colexec/insert/insert.go:265); Reset/Free only abort a writer that is still non-nil (pkg/sql/colexec/insert/types.go:127, pkg/sql/colexec/insert/types.go:159), and externalWriter.Close finalizes the file (pkg/sql/colexec/externalwrite/writer.go:208, pkg/fileservice/file_service_writer.go:86). In a multi-pipeline INSERT/LOAD, one scope can finish and close a complete output file before a sibling scope later hits an expression/read/write error. The statement returns failure, but the already-closed file remains in the stage; retrying the statement then duplicates that scope's rows. The earlier abort-on-live-writer fix covers partial live streams, but not completed sibling streams in the same failed statement. This needs a statement-level commit/abort strategy, or a way to keep produced files temporary until all parallel writers have succeeded.
  3. SHOW CREATE still does not faithfully round-trip LINES TERMINATED BY '\r\n'. The formatter maps both stored "\n" and stored "\r\n" to LINES TERMINATED BY '\\n' (pkg/sql/plan/build_show_util.go:626), while the read/write configs do preserve and honor CRLF (pkg/sql/colexec/external/types.go:277, pkg/sql/compile/operator.go:1023, pkg/sql/colexec/externalwrite/encode.go:333). Replaying SHOW CREATE changes the external-table format from CRLF to LF, so writable tables are still not exact-format recreatable for this supported option.

Also, the latest checks still show Matrixone Utils CI / Coverage failing.

Local verification note: I tried focused tests for csvparser, externalwrite, and plan, but this checkout still fails during build on github.com/unum-cloud/usearch/golang cgo symbols before tests run, so I could not get a local assertion signal.

  1. has been fixed/resolved by another two PRs.
  2. Is by design, there is no reliably way of controlling files generate by failed export. Usually this is left to external apps.
  3. Should be fixed.

Code coverage failure is not related to this PR. I hope OpenAI and Claude will converge :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants