[WIP] PHOENIX-7278 Add support to dump BAD_ROWS in a directory for CsvBulkL… - #2536
Open
xavifeds8 wants to merge 1 commit into
Open
[WIP] PHOENIX-7278 Add support to dump BAD_ROWS in a directory for CsvBulkL…#2536xavifeds8 wants to merge 1 commit into
xavifeds8 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds HDFS bad-record output support for CSV and JSON bulk-load jobs.
Changes:
- Adds
--bad-records-path/-B, implicitly enabling ignored errors. - Writes parser and upsert failures to task-attempt
.badfiles. - Adds option and listener unit tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
BulkLoadToolTest.java |
Tests option parsing. |
BadRecordsWriterTest.java |
Tests bad-record listener behavior. |
FormatToBytesWritableMapper.java |
Implements bad-record file output. |
AbstractBulkLoadTool.java |
Configures the new CLI option. |
Suppressed comments (1)
phoenix-core/src/test/java/org/apache/phoenix/mapreduce/BadRecordsWriterTest.java:25
- This unused import also violates the repository's
UnusedImportscheck (src/main/config/checkstyle/checker.xml:129). Remove it.
import static org.mockito.Mockito.never;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| LOGGER.error("Error on record " + record, throwable); | ||
| context.getCounter(COUNTER_GROUP_NAME, "Errors on records").increment(1L); | ||
| if (badRecordWriter != null) { | ||
| badRecordWriter.write(String.valueOf(record), throwable.getMessage()); |
Comment on lines
+173
to
+176
| String taskAttemptId = context.getTaskAttemptID().toString(); | ||
| Path badRecordFile = new Path(outputDir, taskAttemptId + ".bad"); | ||
| badRecordsWriter = new PrintWriter( | ||
| new OutputStreamWriter(fs.create(badRecordFile, false), StandardCharsets.UTF_8)); |
Comment on lines
+379
to
+381
| if (badRecordsWriter != null) { | ||
| badRecordsWriter.close(); | ||
| } |
| package org.apache.phoenix.mapreduce; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…oadTool
What changes were proposed in this pull request?
Added a --bad-records-path (-B) CLI option to CsvBulkLoadTool / JsonBulkLoadTool that writes rejected/malformed records to an HDFS directory during bulk import. When a record fails parsing (e.g., unterminated CSV quote) or fails upsert (e.g., type mismatch), the offending record and its error message are written to a per-task-attempt file (.bad) under the configured path. This option implies --ignore-errors.
Why are the changes needed?
Previously, when --ignore-errors was used, bad records were silently skipped with only a counter increment. Operators had no way to identify which records failed or why, making it difficult to fix source data and re-import. This change provides an audit trail of rejected records for post-job analysis and data remediation.
Does this PR introduce any user-facing change?
Yes. A new CLI option --bad-records-path / -B is available for CsvBulkLoadTool and JsonBulkLoadTool. When specified, rejected records are written to the given HDFS path. Using this option implicitly enables --ignore-errors.
How was this patch tested?
Unit tests in BadRecordsWriterTest covering MapperUpsertListener error handling, BadRecordWriter interface, null writer safety, counter increments, error propagation behavior, and multi-error scenarios (10 tests, all passing).
Existing BulkLoadToolTest.testParseOptions_BadRecordsPath verifies option parsing.
Was this patch authored or co-authored using generative AI tooling?
Yes using claude code