You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "nanditajayanthi (via GitHub)" <gi...@apache.org> on 2024/04/18 21:45:34 UTC
[PR] Add assert [pinot]
nanditajayanthi opened a new pull request, #12966:
URL: https://github.com/apache/pinot/pull/12966
Added assert statements to testParquetFile(String filePath) to ensure that test fails/passes are noted correctly
#12965
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add assert [pinot]
Posted by "nanditajayanthi (via GitHub)" <gi...@apache.org>.
nanditajayanthi commented on code in PR #12966:
URL: https://github.com/apache/pinot/pull/12966#discussion_r1581272631
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/test/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReaderFullTest.java:
##########
@@ -123,19 +123,22 @@ protected void testReadDataSet2()
//testParquetFile("test-data2/uniform_encryption.parquet.encrypted");
}
- protected void testParquetFile(String filePath)
+protected void testParquetFile(String filePath)
throws Exception {
File dataFile = new File(URLDecoder.decode(getClass().getClassLoader().getResource(filePath).getFile(), "UTF-8"));
ParquetNativeRecordReader recordReader = new ParquetNativeRecordReader();
recordReader.init(dataFile, null, null);
+ int counter = 0;
Review Comment:
For this PR, I can count `numDocsForFirstPass` and `numDocsForSecondPass` explicitly. This PR is mainly intended to add an assertion mechanism. Is it still necessary to use the try-with-resource in this PR?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add assert [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12966:
URL: https://github.com/apache/pinot/pull/12966#discussion_r1571478256
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/test/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReaderFullTest.java:
##########
@@ -123,19 +123,22 @@ protected void testReadDataSet2()
//testParquetFile("test-data2/uniform_encryption.parquet.encrypted");
}
- protected void testParquetFile(String filePath)
+protected void testParquetFile(String filePath)
Review Comment:
Please fix the format
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/test/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReaderFullTest.java:
##########
@@ -123,19 +123,22 @@ protected void testReadDataSet2()
//testParquetFile("test-data2/uniform_encryption.parquet.encrypted");
}
- protected void testParquetFile(String filePath)
+protected void testParquetFile(String filePath)
throws Exception {
File dataFile = new File(URLDecoder.decode(getClass().getClassLoader().getResource(filePath).getFile(), "UTF-8"));
ParquetNativeRecordReader recordReader = new ParquetNativeRecordReader();
recordReader.init(dataFile, null, null);
+ int counter = 0;
Review Comment:
Would it be more explicit if we count `numDocsForFirstPass` and `numDocsForSecondPass` and compare them? Can you also use try-with-resource to close the record reader (not introduced in this PR)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add assert [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed pull request #12966: Add assert
URL: https://github.com/apache/pinot/pull/12966
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add assert [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12966:
URL: https://github.com/apache/pinot/pull/12966#discussion_r1581326170
##########
pinot-plugins/pinot-input-format/pinot-parquet/src/test/java/org/apache/pinot/plugin/inputformat/parquet/ParquetNativeRecordReaderFullTest.java:
##########
@@ -123,19 +123,22 @@ protected void testReadDataSet2()
//testParquetFile("test-data2/uniform_encryption.parquet.encrypted");
}
- protected void testParquetFile(String filePath)
+protected void testParquetFile(String filePath)
throws Exception {
File dataFile = new File(URLDecoder.decode(getClass().getClassLoader().getResource(filePath).getFile(), "UTF-8"));
ParquetNativeRecordReader recordReader = new ParquetNativeRecordReader();
recordReader.init(dataFile, null, null);
+ int counter = 0;
Review Comment:
E.g when the assert fails, it will leak resource because the last `close()` won't be executed. Using try-with-resource is just good practice to prevent resource leak
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org