Repository navigation
Fix import-data and export-data correctness and error reporting - #18828
Conversation
|
I reviewed commit
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 |
CritasWang
left a comment
There was a problem hiding this comment.
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-tsfilestill has the archival bug this PR fixes forimport-data.ImportTsFileBase#processingFilestill computesisSuccess ? successDir : failDir + File.separator + ..., so every successfully loaded file is archived to the success directory path itself. With-os mvand 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 newprocessingFile, either here or in a quick follow-up.export-tsfile.bat,schema/export-schema.batandschema/import-schema.batstill haveENDLOCALandEXIT /B %ret_code%on separate lines. That is the same exit-code problem fixed here forexport-data.bat/import-data.bat.
Nits
- The three new
CliMessagesconstants 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,
processedRowsnow 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
NULLis accepted by the treeINSERTgrammar and becomes a real null on the server (CommonUtils.parseValue). identifierValuelower-cases names the same way the server canonicalizes them (QualifiedName).iotdb-relational-grammarwas already on the CLI classpath throughnode-commons, so the packaged CLI does not change.
| public abstract class AbstractImportData extends AbstractDataTool implements Runnable { | ||
|
|
||
| private static final IoTPrinter ioTPrinter = new IoTPrinter(System.out); | ||
| private static final AtomicBoolean importFailed = new AtomicBoolean(); |
There was a problem hiding this comment.
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:
.sqlinput. Failed statements inImportDataTree#importFromSqlFile/ImportDataTable#importFromSqlFileare only written to the.failedfile. In a probe where every statement throwsStatementExecutionException,init()still returnstrue.- CSV files rejected as a whole. This covers an empty file, a first column that is not
Time, a wrong extension, and anIOExceptionwhile reading, i.e. the early returns and thecatchin bothimportFromCsvFileimplementations. A probe with the headerts,root.a.d1.s1returnstrue.
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.
There was a problem hiding this comment.
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.
|
Applied both findings in
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. 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. |
|
Applied the remaining review feedback in
Validation: Spotless and CLI Checkstyle passed. Full-reactor No live-cluster integration tests were run. Review threads are left open for re-review. |
CritasWang
left a comment
There was a problem hiding this comment.
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.
CritasWang
left a comment
There was a problem hiding this comment.
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.
-qtogether 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.
…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
…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.
Description
import-dataandexport-datacan 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:
NULLfor missing values..resourceand.modssidecars, and reporting file-operation failures accurately.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.ret_codeunset, 0, and 23.git diff --checkpassed.No live-cluster integration tests were run.
This PR has: