Skip to content

Fix import-data and export-data correctness and error reporting - #18828

Merged
CritasWang merged 7 commits into
apache:masterfrom
Caideyipi:fix/data-import-export-correctness
Oct 22, 2026
Merged

CritasWang merged 7 commits into
apache:masterfrom
Caideyipi:fix/data-import-export-correctness

Conversation

@Caideyipi

Copy link
Copy Markdown
Collaborator

Description

import-data and export-data can silently produce incorrect output or report success after an import fails. For example, a tree query containing interleaved columns from two devices currently exports only the first device and assigns it a value from the other device. Multiple custom table queries also overwrite the same output file and can use the wrong destination table.

This change fixes the data tools by:

  • Preserving the original column indices for every tree device, omitting absent values, and splitting SQL files by the number of generated INSERT statements.
  • Giving each custom table query its own output files and deriving the destination table with the existing relational SQL parser. SQL/TsFile queries with multiple source relations or a different database fail before writing. Table and column identifiers are quoted.
  • Escaping SQL text literals and emitting SQL NULL for missing values.
  • Honoring typed CSV export headers and retaining the original CSV header spelling when importing table data with case-insensitive names and types.
  • Clearing CSV batches only after successful insertion and propagating exhausted retries, worker errors, and archive errors to a nonzero process exit code.
  • Archiving each imported TsFile under its relative filename with its .resource and .mods sidecars, and reporting file-operation failures accurately.
  • Preserving the Java process exit code in the Windows import/export launchers.

Validation

  • mvn -o test -pl iotdb-client/cli -Dtest=DataToolRegressionTest,ImportTsFileOperationTest,WriteDataFileTest -Ddevelocity.off=true: 21 tests passed, including 18 new regression tests.
  • mvn -o test-compile -DskipTests -Ddevelocity.off=true: full 53-module reactor passed.
  • mvn -o test-compile -P with-zh-locale -DskipTests -Ddevelocity.off=true: full 53-module Chinese-locale reactor passed.
  • Windows launcher probes: both scripts returned Java exit code 17 with inherited ret_code unset, 0, and 23.
  • git diff --check passed.

No live-cluster integration tests were run.

This PR has:

  • been self-reviewed.
  • added regression tests for the corrected behavior.

@CritasWang

Copy link
Copy Markdown
Collaborator

I reviewed commit e5db31f1a860c19ba1a872f0b222e0612f5bb746 and reproduced two issues that should be addressed before merging:

  1. [P1] Archive filename collisions can silently discard a source file.

    In AbstractImportData.java, lines 166–180, replacing every path separator with _ is not an injective mapping. For example, input/a/b.tsfile and input/a_b.tsfile both become <successDir>/a_b.tsfile. The subsequent file operations use REPLACE_EXISTING, so the second archive operation silently overwrites the first. With -os mv, both original paths disappear, but only one file remains in the archive, and no error is propagated. A local regression test expecting both files to survive instead found only one. Please preserve the relative directory structure, or use collision-safe names and reject unintended overwrites.

  2. [P2] Typed CSV export breaks round trips for column names containing spaces.

    This PR starts honoring typed CSV headers, but ImportDataTable.java, lines 395–401 still removes all whitespace from the column name when parsing a typed header. For a legal column named value with space, -dt true exports value with space(DOUBLE), which the importer turns into valuewithspace. Tablet.addValue then throws an IllegalArgumentException because that measurement does not exist. I reproduced this by exporting and importing the same row: the untyped CSV imported successfully, while the typed CSV failed. Please strip only the type suffix and preserve whitespace within the identifier.

Validation was performed in an isolated checkout: the PR's existing 21 targeted tests passed, and two additional regression tests reproduced the issues above. The tests used the locally cached TsFile 2.4.1-261009-SNAPSHOT because 2.4.1-260915-SNAPSHOT was unavailable locally. No live-cluster integration tests were run.

@CritasWang CritasWang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for working on this. I went through the full diff at e5db31f together with the surrounding code. The tree multi-device SQL export, the per-query output files and destination-table derivation, literal escaping and NULL handling, the typed CSV headers, and the archival target fix all look correct to me. The 21 tests in DataToolRegressionTest, ImportTsFileOperationTest and WriteDataFileTest pass locally, and I reproduced the import-side issues below with small unit probes.

I'd like the first inline comment addressed before merging: a failed CSV batch now ends the whole worker thread, so the files queued behind it are skipped and never reported. The second is about the exit-code coverage that is still incomplete. The rest is minor.

Follow-ups outside this diff

  • import-tsfile still has the archival bug this PR fixes for import-data. ImportTsFileBase#processingFile still computes isSuccess ? successDir : failDir + File.separator + ..., so every successfully loaded file is archived to the success directory path itself. With -os mv and a fresh (empty) success directory, this deletes source files. The first loaded TsFile replaces the empty directory, and each later one replaces the previous file (REPLACE_EXISTING). All of them have already been moved out of the source directory, so only the last one survives on disk. It is the same logic, so it could reuse the new processingFile, either here or in a quick follow-up.
  • export-tsfile.bat, schema/export-schema.bat and schema/import-schema.bat still have ENDLOCAL and EXIT /B %ret_code% on separate lines. That is the same exit-code problem fixed here for export-data.bat / import-data.bat.

Nits

  • The three new CliMessages constants sit between the private constructor and the "Additional auto-collected messages" separator. Their names and hashes are correct; only the placement looks accidental.
  • In the tree non-aligned SQL export, processedRows now counts INSERT statements rather than result rows, so the progress number means something slightly different.

Checked, no action needed

  • '' escaping is valid for both the tree and the relational lexer; neither treats \ as an escape.
  • Unquoted NULL is accepted by the tree INSERT grammar and becomes a real null on the server (CommonUtils.parseValue).
  • identifierValue lower-cases names the same way the server canonicalizes them (QualifiedName).
  • iotdb-relational-grammar was already on the CLI classpath through node-commons, so the packaged CLI does not change.

Comment thread iotdb-client/cli/src/main/java/org/apache/iotdb/tool/data/AbstractImportData.java Outdated
public abstract class AbstractImportData extends AbstractDataTool implements Runnable {

private static final IoTPrinter ioTPrinter = new IoTPrinter(System.out);
private static final AtomicBoolean importFailed = new AtomicBoolean();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

importFailed is private and is only set inside AbstractImportData: on worker exceptions, archival failures and failed TsFile loads. These paths still print "Import completely!" and exit with 0:

  • .sql input. Failed statements in ImportDataTree#importFromSqlFile / ImportDataTable#importFromSqlFile are only written to the .failed file. In a probe where every statement throws StatementExecutionException, init() still returns true.
  • CSV files rejected as a whole. This covers an empty file, a first column that is not Time, a wrong extension, and an IOException while reading, i.e. the early returns and the catch in both importFromCsvFile implementations. A probe with the header ts,root.a.d1.s1 returns true.

Could we add something like protected static void markImportFailed() and call it from those branches? If that is out of scope on purpose, it would help to state in the description which failure modes now produce a non-zero exit code.

Whether row-level conversion failures that only go to .failed should also fail the run is a separate product decision, so I'm not suggesting that here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 240cfd502cce. Shared failure markers now cover failed SQL statements, table SQL database mismatches, SQL read errors, and CSV files rejected for an empty file, invalid time header, unsupported extension, or read/path errors. Failed SQL files are counted once as failures instead of successes. Added coverage in both models. The existing row-level conversion/.failed policy is unchanged.

@Caideyipi

Copy link
Copy Markdown
Collaborator Author

Applied both findings in faa5d5e5ed0.

  1. TsFile archival now preserves the source's relative directory structure, so a/b.tsfile and a_b.tsfile remain distinct in both success and failure archives. It checks the main file and existing sidecar destinations before processing the group, rejects occupied targets, and no longer uses REPLACE_EXISTING for copies, moves, or hard-link fallbacks. Archival errors propagate to the import result.
  2. Table CSV header parsing now removes only the trailing type annotation and preserves whitespace and embedded parentheses in the column name.

Added regression coverage for the colliding paths under copy/hard-link/move operations, existing main-file and sidecar conflicts without source-file loss, and typed/untyped CSV export-import round trips with a column containing spaces and parentheses.

Validation: Spotless and CLI Checkstyle passed. mvn -o test -pl iotdb-client/cli -Dtest=DataToolRegressionTest passed all 21 tests, and the same command with -P with-zh-locale also passed all 21 tests. These runs used the PR's declared TsFile 2.4.1-260915-SNAPSHOT.

Full-reactor checks remain incomplete due to local memory limits: the English build failed with a JVM native-memory allocation error in Thrift compilation; the Chinese retry was stopped during DataNode Checkstyle with its capped 512 MB heap full. No live-cluster integration tests were run.

@Caideyipi

Copy link
Copy Markdown
Collaborator Author

Applied the remaining review feedback in 240cfd502cce:

  • Worker exceptions are handled per file in both queues, so a failed CSV batch no longer skips the files behind it. SQL statement/read failures and whole-file CSV rejections now fail the run and update file counts.
  • import-tsfile now shares the collision-safe TsFile/sidecar operations with import-data. Both success and failure archives preserve relative directories, reject occupied main/sidecar targets before moving the group, and propagate archival errors to the processing result.
  • Fixed return-code propagation in export-tsfile.bat, schema/export-schema.bat, and schema/import-schema.bat.
  • Documented table SQL/TsFile custom-query limitations in both locales, grouped the new message constants before the constructor, and restored result-row counting for non-aligned tree SQL export progress.

Validation: Spotless and CLI Checkstyle passed. mvn -o test -pl iotdb-client/cli -Dtest=DataToolRegressionTest,ImportTsFileOperationTest,WriteDataFileTest passed all 31 tests, and the same command with -P with-zh-locale also passed all 31. These runs used the declared TsFile 2.4.1-260915-SNAPSHOT. The 51 English/Chinese message keys and format specifiers match. All five Windows launchers touched by this PR correctly returned a Java probe's exit code 7 despite a caller-side ret_code=91.

Full-reactor mvn -o test-compile -DskipTests and mvn -o test-compile -DskipTests -P with-zh-locale both passed all 53 modules, with Maven capped at a 1 GB heap.

No live-cluster integration tests were run. Review threads are left open for re-review.

@CritasWang CritasWang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed the full diff at aa620ee. Everything from my previous review is addressed: worker loops now handle failures per file, SQL statement and whole-file CSV failures set the exit code, import-tsfile shares the collision-safe processFileGroup and now returns a real status, the three extra Windows launchers propagate ret_code, and the -q help text documents the custom-query limits. The en/zh CliMessages keys match and the new constant hashes are correct.

Locally (isolated checkout; TsFile 2.4.1-261009-SNAPSHOT because 2.4.1-260915-SNAPSHOT isn't available offline), DataToolRegressionTest, ImportTsFileOperationTest, WriteDataFileTest and ImportTsFileTest pass: 39/39.

Two new comments inline. The first one is a silent data-loss case that aa620ee introduced in the typed-CSV round trip; the second one is minor.

No live-cluster integration tests were run.

Comment thread iotdb-client/cli/src/main/java/org/apache/iotdb/tool/data/ExportDataTable.java Outdated

@CritasWang CritasWang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed aa620ee..0ccd98b. Both findings from my last review are fixed, and I found no new issues.

  • Ambiguous table CSV headers. Header resolution now runs in two passes: exact column names first, then type suffixes on the remaining headers. Any duplicate target column rejects the whole file before insertion and counts it as a failure. The new constant's hash is correct, and the en/zh keys match.
  • -q together with -table. Custom-query validation uses the full table list, and automatic export still filters by -table.

Locally (isolated checkout, TsFile 2.4.1-261009-SNAPSHOT), DataToolRegressionTest, ImportTsFileOperationTest, WriteDataFileTest and ImportTsFileTest pass, 43/43. An extra probe for exact/typed/case-only duplicate headers also passes. spotless:check and the CLI -P with-zh-locale compile pass. No live-cluster integration tests were run.

LGTM from my side.

@CritasWang
CritasWang merged commit 0acff49 into apache:master Oct 22, 2026
39 checks passed
@Caideyipi
Caideyipi deleted the fix/data-import-export-correctness branch October 22, 2026 06:01
VGalaxies pushed a commit to VGalaxies/iotdb that referenced this pull request Oct 26, 2026
…he#18828)

* Fix import-data and export-data correctness and error reporting

* Declare relational grammar dependency for CLI export

* Fix Windows relative source archival test

* Fix archive collisions and typed CSV column names

* Handle import failures per file and share safe TsFile archival

* Fix standalone TsFile import status and parenthesized CSV columns

* Fix ambiguous table CSV imports and custom query validation
CritasWang added a commit that referenced this pull request Oct 27, 2026
…18987)

* Add measurement files, time ranges, progress and logs to export-data

- -mf/--measurement_file exports the measurements matched by the IoTDB path
  patterns in a file, one output per measurement named by its full path.
  The whole file is validated before any query, and each unmatched pattern
  is reported with its line.
- -start_time/-end_time form an inclusive interval for measurement-file and
  automatic Table exports. Queries run as written and report the ignored
  bounds as a WARNING.
- Progress is reported every two seconds: one redrawn line in a terminal,
  one line per update otherwise, with an ETA in measurement-file mode.
- Every run appends a JSON Lines log, <target>/log_export_data_<time>.log
  by default or the path given with -l/--export_data_log, ending with one
  FINISH event. The console ends with the summary, after the log path.
- CSV and SQL files are split by data rows; the INSERT statements of one
  row stay in one file.
- Ctrl+C and SIGTERM stop at the next row, close the query and the session
  and exit with 130 or 143; failures exit with 1.
- A Table query exported as SQL or TsFile still writes to the table it
  reads, as in #18828; -table is ignored with a WARNING, and every query is
  checked before the first one runs.

* Check the columns of Table SQL/TsFile export queries

- A Table query exported as SQL or TsFile must select * or column names
  from one table of -db. Aliases, expressions and FILL would write values
  the table does not hold under its column names, so they are rejected,
  as are EXPLAIN, set operations, column aliases and subqueries without a
  table.
- The query must return the time column and at least one FIELD column,
  each column once; TsFile queries also need every TAG column. The
  columns are checked against the table before the first query runs and
  again against the columns the query returns.
- Export option descriptions move to Constants, keeping their lines within
  100 characters, and the -q and -db help text is shortened.

* Read Table TsFile export values through a TypeService

The repository architecture test allows switches on TSDataType only
inside a TypeService, and ExportDataTable.exportTsFile switched on the
column type for every value. It now resolves one reader per column from
a TypeService, keeping the same getters: getLong for TIMESTAMP, getDate
for DATE, getBlob for BLOB and getObject for the other types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants