COLDMANYU commented on code in PR #10824:
URL: https://github.com/apache/paimon/pull/10824#discussion_r4386003027
##########
paimon-format/src/main/java/org/apache/paimon/format/csv/CsvFileReader.java:
##########
@@ -55,7 +57,13 @@ public CsvFileReader(
}
@Override
+ @Nullable
protected InternalRow parseLine(String line) {
+ // Same test as Spark's CSV source (String#trim drops chars <=
U+0020). Returning null
+ // makes the text reader move on to the next line.
+ if (skipEmptyLines && line.trim().isEmpty()) {
Review Comment:
Thanks for the careful review and the reproduction! You're right, I only had
external format tables in mind and missed that a normal file-store table
accepts the option too.
I chose to reject it for persisted tables. Our use case only needs it for
external format tables that read CSV files written by Spark, so restricting the
option keeps the change small and avoids touching metadata aggregation. If
there is a use case for file-store tables later, it could be supported then by
disabling metadata aggregation wherever rows can be skipped.
- `SchemaValidation.validateTableSchema` now rejects
`csv.skip-empty-lines=true` unless the table type is `format-table`. This check
also runs when an `ALTER TABLE` sets the option and when the option is passed
as a dynamic option through `Table#copy`, so none of these paths can turn it on
for a file-store table.
- Format tables are not affected by the count issue:
`FormatDataSplit#mergedRowCount()` is always empty, so
`AggregatePushDownUtils.tryPushdownAggregation` never pushes `COUNT(*)` down
for them and the rows are always scanned.
- The option description and the docs now say that it is only supported for
format tables.
New table-level tests in `CsvSkipEmptyLinesTableTest`:
- Creating a file-store table with the option, setting it with `ALTER
TABLE`, and passing it as a dynamic option all fail.
- With your setup (single nullable INT column, `csv`, `gzip`, `bucket=-1`,
rows `(NULL), (1)`), a scan without the option returns 2 rows and the metadata
row count is also 2.
- A CSV format table with the option skips the empty lines, and its splits
report no row count.
Without the new check, the three rejection tests fail. A scan with the
option then returned 1 row while the metadata still reported 2, the same
mismatch you found.
--
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]
