Set header and footer skip line count for CSV tables #1090

Praveen2112 · 2019-07-06T14:23:28Z

Fixes #1085 . Here we replace text_skip_header_line_count and text_skip_footer_line_count to skip_header_line_count and skip_footer_line_count which would be common for both csv and text file.

kokosing

Please also add a test where data for csv file spans across two splits to see if data read by Presto is correct.

kokosing · 2019-07-07T18:31:20Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

-        // Textfile specific property
-        String textSkipHeaderCount = table.get().getParameters().get(TEXT_SKIP_HEADER_COUNT_KEY);
+        // Textfile and CSV specific property
+        String textSkipHeaderCount = table.get().getParameters().get(SKIP_HEADER_COUNT_KEY);


rename variable to skipHeaderCount

kokosing · 2019-07-07T18:31:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

        }
-        String textSkipFooterCount = table.get().getParameters().get(TEXT_SKIP_FOOTER_COUNT_KEY);
+        String textSkipFooterCount = table.get().getParameters().get(SKIP_FOOTER_COUNT_KEY);


kokosing · 2019-07-07T18:33:12Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

            if (headerSkipCount > 0) {
-                checkFormatForProperty(hiveStorageFormat, HiveStorageFormat.TEXTFILE, TEXTFILE_SKIP_HEADER_LINE_COUNT);
-                tableProperties.put(TEXT_SKIP_HEADER_COUNT_KEY, String.valueOf(headerSkipCount));
+                checkFormatForProperty(hiveStorageFormat, expectedFormats, SKIP_HEADER_LINE_COUNT);


Maybe:

checkFormatForProperty(hiveStorageFormat, HiveStorageFormat.TEXTFILE, TEXTFILE_SKIP_HEADER_LINE_COUNT); checkFormatForProperty(hiveStorageFormat, HiveStorageFormat.CSV, TEXTFILE_SKIP_HEADER_LINE_COUNT); ```?

But a csv format would fail in first statement and similarly for text format.

kokosing · 2019-07-07T18:33:42Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

-        // Textfile specific properties
-        getTextHeaderSkipCount(tableMetadata.getProperties()).ifPresent(headerSkipCount -> {
+        // Textfile and CSV specific properties
+        ImmutableSet<HiveStorageFormat> expectedFormats = ImmutableSet.of(HiveStorageFormat.TEXTFILE, HiveStorageFormat.CSV);


expectedFormats -> csvAndTextFile

kokosing · 2019-07-07T18:35:52Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

-        if (tableStorageFormat == HiveStorageFormat.TEXTFILE) {
-            if (table.getParameters().containsKey(TEXT_SKIP_HEADER_COUNT_KEY)) {
-                throw new PrestoException(NOT_SUPPORTED, format("Inserting into Hive table with %s property not supported", TEXT_SKIP_HEADER_COUNT_KEY));
+        if (tableStorageFormat == HiveStorageFormat.TEXTFILE || tableStorageFormat == HiveStorageFormat.CSV) {


I wonder if this wrapping if block is needed here.

kokosing · 2019-07-07T18:39:58Z

presto-product-tests/src/main/resources/csv/data.csv

@@ -0,0 +1,3 @@
+header1,header2,header3
+value1,value2,value3
+footer1,footer2,footer3


no new line at the end

kokosing · 2019-07-07T18:40:57Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+        onHive().executeQuery("DROP TABLE test_csvfile_skip_header");
+
+        onHive().executeQuery("DROP TABLE IF EXISTS test_csvfile_skip_footer");
+        onHive().executeQuery("" +


can you divide this test into two tests

kokosing · 2019-07-07T18:42:10Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

@@ -164,6 +192,87 @@ private void testCreatePartitionedCsvTableAs(String tableName, String additional
        query("DROP TABLE " + tableName);
    }

+    @Test
+    public void testCreateTextFileSkipHeaderFooter()


can you please migrate this test to convention tests. See:

cox@Grzegorzs-MacBook-Pro:~/presto/presto-product-tests$ find src -name csv* src/main/resources/sql-tests/datasets/csv_table.ddl src/main/resources/sql-tests/datasets/csv_table_with_custom_parameters.ddl src/main/resources/sql-tests/datasets/csv_table_with_custom_parameters.data src/main/resources/sql-tests/datasets/csv_table.data src/main/resources/sql-tests/testcases/csv_table_with_custom_parameters.sql src/main/resources/sql-tests/testcases/csv_table.sql

kokosing · 2019-07-07T18:46:47Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java

-                        "   textfile_skip_footer_line_count = 1,\n" +
-                        "   textfile_skip_header_line_count = 1\n" +
+                        "   skip_footer_line_count = 1,\n" +
+                        "   skip_header_line_count = 1\n" +


can you please add tests for csv with skip_header_line_count and skip_footer_line_count here?

martint · 2019-08-16T23:13:10Z

@Praveen2112, any updates on this?

Praveen2112 · 2019-08-17T09:39:07Z

@martint Yes on the final leg of testing...

Praveen2112 · 2019-08-18T11:54:17Z

@kokosing , @martint Have applied the comments

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

kokosing · 2019-08-19T05:19:32Z

presto-product-tests/src/main/resources/csv/data.csv

@@ -0,0 +1,3 @@
+header1,header2,header3


this is very small data test... maybe let's inline this into the java file?

kokosing · 2019-08-19T05:20:36Z

presto-product-tests/src/main/resources/csv/data.csv

@@ -0,0 +1,3 @@
+header1,header2,header3
+value1,value2,value3
+footer1,footer2,footer3


footer and header does not need to have same number of columns. It is a bit misleading because it looks like data.

#### HEADER ####### value1,value2,value3 #### FOOTER #######

?

kokosing · 2019-08-19T05:21:53Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java

+        HiveSplitSource hiveSplitSource = hiveSplitSource(backgroundHiveSplitLoader);
+        backgroundHiveSplitLoader.start(hiveSplitSource);
+
+        assertEquals(drainSplits(hiveSplitSource).size(), 33);


❤️

Maybe we can now port tests that were using ./src/main/resources/sql-tests/datasets/table_with_header_and_footer.data-generator?

kokosing · 2019-08-19T05:24:49Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveTableProperties.java

@@ -129,8 +129,8 @@ public HiveTableProperties(TypeManager typeManager, HiveConfig config)
                        false),
                integerProperty(BUCKET_COUNT_PROPERTY, "Number of buckets", 0, false),
                stringProperty(AVRO_SCHEMA_URL, "URI pointing to Avro schema for the table", null, false),
-                integerProperty(TEXTFILE_SKIP_HEADER_LINE_COUNT, "Number of header lines", null, false),
-                integerProperty(TEXTFILE_SKIP_FOOTER_LINE_COUNT, "Number of footer lines", null, false),
+                integerProperty(SKIP_HEADER_LINE_COUNT, "Number of header lines", null, false),


@electrum @findepi do you guys like his rename of textfile_skip_header_line_count to skip_footer_line_count hive table property?

kokosing · 2019-08-20T17:18:42Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+    @Inject
+    private HdfsClient hdfsClient;
+
+    @BeforeTestWithContext


This executes before each test.

The best would be to use test requirements here. So you could define HiveTableDefinition that would automatically handle data upload. See io.prestosql.tests.hive.TestHivePartitionsTable#getRequirements and usage of io.prestosql.tempto.fulfillment.table.hive.InlineDataSource#createStringDataSource

That way you don't need to clean this up, because tempto will take care of this.

Also IIRC tables that are defined in resources/sql-tests/datasets/ should be also available here via ImmutableTablesState so you would need to setup anything here.

kokosing · 2019-08-20T17:21:33Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+    @Test
+    public void testCreateCsvFileSkipHeader()
+    {
+        onHive().executeQuery("DROP TABLE IF EXISTS test_create_csvfile_skip_header");


why on Hive? let's use Presto whenever possible

kokosing · 2019-08-20T17:29:43Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+    }
+
+    @Test
+    public void testInsertCsvSkipHeader()


can you merge test inserts with create table tests. Tempto tests are a bit heavy.

kokosing · 2019-08-20T17:32:25Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+                "WITH ( " +
+                "   format = 'CSV', " +
+                "   external_location = 'hdfs://hadoop-master:9000/user/hive/warehouse/TestCsvFileHiveTable/csv_table_with_header', " +
+                "   csv_separator= ','," +


can you test different separator like %? So we could see that it works with different separators.

kokosing · 2019-08-20T17:36:24Z

presto-product-tests/src/main/resources/sql-tests/datasets/csv_with_header_footer.ddl

+LOCATION '%LOCATION%'
+TBLPROPERTIES (
+    'skip.header.line.count' = '1',
+    'skip.footer.line.count' = '1')


I don't see tests that are using these tables. See:

./presto-product-tests/src/main/resources/sql-tests/testcases/csv_table_with_custom_parameters.sql ./presto-product-tests/src/main/resources/sql-tests/testcases/csv_table.sql

If you provide convention tests like the ones mentioned above then you can remove all select tests from TestCsv and leave there only tests for inserts. Or even you could remove tests for insert from there because that could be easily tested in TestHiveIntegrationSmokeTest

kokosing

Just few minor comments otherwise look good.

kokosing · 2019-10-09T05:52:13Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

@@ -164,6 +172,24 @@ private void testCreatePartitionedCsvTableAs(String tableName, String additional
        query("DROP TABLE " + tableName);
    }

+    @Test


@Test(groups = {STORAGE_FORMATS})

kokosing · 2019-10-09T05:53:33Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestCsv.java

+    @Test
+    public void testCreateCsvFileSkipHeader()
+    {
+        assertThat(query("SELECT * FROM csv_with_header")).containsOnly(row("10", "value2"));


This could be easily refactored to tempto convention tests.

Have you seen:./src/main/resources/sql-tests/testcases/tables_with_header_and_footer.sql?

This way it is able to change tests and then execute them without recompiling.

kokosing · 2019-10-09T05:56:46Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java

+        HiveSplitSource hiveSplitSource = hiveSplitSource(backgroundHiveSplitLoader);
+        backgroundHiveSplitLoader.start(hiveSplitSource);
+
+        assertEquals(drainSplits(hiveSplitSource).size(), 1);


can you please extract method like:

assertSplitsCount(CSV, ImmutableMap.of("skip.header.line.count", "1", "skip.footer.line.count", "1"), 1 /** splits count */);

I see that all the tests you added looked very similar.

kokosing · 2019-10-09T05:57:10Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java

@@ -49,6 +49,7 @@
 import java.util.ArrayList;


I would merge this commit with the previous one.

kokosing · 2019-10-09T07:40:03Z

...o-product-tests/src/main/resources/sql-tests/testcases/csv_tables_with_header_and_footer.sql

@@ -0,0 +1,13 @@
+-- database: presto; tables: csv_with_header, csv_with_footer, csv_with_header_and_footer; groups: hive, hive_file_header;


use groups: storage_formats

cla-bot bot added the cla-signed label Jul 6, 2019

Praveen2112 force-pushed the header_footer_csv branch 5 times, most recently from c18e425 to ba1c84f Compare July 7, 2019 14:22

kokosing reviewed Jul 7, 2019

View reviewed changes

dain assigned Praveen2112 Aug 1, 2019

Praveen2112 force-pushed the header_footer_csv branch from ba1c84f to c5fc261 Compare August 18, 2019 11:51

Praveen2112 force-pushed the header_footer_csv branch 3 times, most recently from a16fa5c to ef52942 Compare August 19, 2019 04:47

kokosing approved these changes Aug 19, 2019

View reviewed changes

kokosing reviewed Aug 19, 2019

View reviewed changes

Praveen2112 force-pushed the header_footer_csv branch from ef52942 to 6babaf2 Compare August 20, 2019 14:36

kokosing reviewed Aug 20, 2019

View reviewed changes

Praveen2112 force-pushed the header_footer_csv branch from 6babaf2 to d202a9d Compare October 8, 2019 11:20

Rename skip header and footer line property for text file

5533984

Praveen2112 force-pushed the header_footer_csv branch 3 times, most recently from 2a9412a to 336d1cc Compare October 8, 2019 17:03

kokosing reviewed Oct 9, 2019

View reviewed changes

Praveen2112 force-pushed the header_footer_csv branch from 336d1cc to 91603e2 Compare October 9, 2019 07:33

kokosing approved these changes Oct 9, 2019

View reviewed changes

Praveen2112 force-pushed the header_footer_csv branch 2 times, most recently from 172583f to ead33aa Compare October 9, 2019 08:10

Add header and footer skip line count for CSV tables

16c6784

Praveen2112 force-pushed the header_footer_csv branch from ead33aa to 16c6784 Compare October 9, 2019 09:46

kokosing merged commit 4514c6f into trinodb:master Oct 9, 2019

kokosing mentioned this pull request Oct 9, 2019

Release notes for 321 #1681

Closed

6 tasks

martint added this to the 321 milestone Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set header and footer skip line count for CSV tables #1090

Set header and footer skip line count for CSV tables #1090

Praveen2112 commented Jul 6, 2019

kokosing left a comment

kokosing Jul 7, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

Praveen2112 Aug 17, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

kokosing Jul 7, 2019

martint commented Aug 16, 2019

Praveen2112 commented Aug 17, 2019

Praveen2112 commented Aug 18, 2019

kokosing Aug 19, 2019

kokosing Aug 19, 2019

kokosing Aug 19, 2019

kokosing Aug 19, 2019

kokosing Aug 20, 2019

kokosing Aug 20, 2019

kokosing Aug 20, 2019

kokosing Aug 20, 2019

kokosing Aug 20, 2019

kokosing left a comment

kokosing Oct 9, 2019

kokosing Oct 9, 2019

kokosing Oct 9, 2019

kokosing Oct 9, 2019

kokosing Oct 9, 2019

		@@ -0,0 +1,13 @@
		-- database: presto; tables: csv_with_header, csv_with_footer, csv_with_header_and_footer; groups: hive, hive_file_header;

Set header and footer skip line count for CSV tables #1090

Set header and footer skip line count for CSV tables #1090

Conversation

Praveen2112 commented Jul 6, 2019

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint commented Aug 16, 2019

Praveen2112 commented Aug 17, 2019

Praveen2112 commented Aug 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment