You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "deshanxiao (via GitHub)" <gi...@apache.org> on 2023/01/30 10:19:55 UTC
[GitHub] [orc] deshanxiao opened a new pull request, #1387: ORC-1362:Enable ENABLE_INDEXES [Draft]
deshanxiao opened a new pull request, #1387:
URL: https://github.com/apache/orc/pull/1387
### What changes were proposed in this pull request?
This PR is aimed to enable ENABLE_INDEXES again.
### Why are the changes needed?
In https://github.com/apache/orc/pull/1373, we disable the configuration "orc.create.index" because the orc reader cannot work well when orc.create.index=false.
The reason why the reader cannot work well because it depends the value of orc.row.index.stride read from orc file. When orc.create.index=false and orc.row.index.stride > 0, the value of orc.create.index will be written to the orc file but the value of orc.row.index.stride will not. When we create a searcher, it will read all the index and then it fails for the empty index.
In the process of pickupgroups, if a failure occurs, we should not just exit but read the file with no index.
This PR also introduce a new limitation:
If orc.create.index=false, it will throw a exception if orc.row.index.stride > 0.
### How was this patch tested?
UT
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437671482
Thank you so much, @deshanxiao , @guiyanakuang , @pavibhai !
Also, cc @wgtmac , @stiga-huang , @coderex2522 for C++ part compatibility. Please let us know if there is any concern for this.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai merged pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai merged PR #1387:
URL: https://github.com/apache/orc/pull/1387
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on a diff in pull request #1387: ORC-1362:Enable ENABLE_INDEXES
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1099619062
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,37 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ Path[] paths = new Path[] {
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexCPlusPlus.orc"),
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexJava.orc"),
Review Comment:
Please add some descriptions of the test file columns.
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,37 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ Path[] paths = new Path[] {
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexCPlusPlus.orc"),
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexJava.orc"),
+ };
+ for (Path path: paths) {
+ FileSystem fs = path.getFileSystem(conf);
+ try (ReaderImpl reader = (ReaderImpl) OrcFile.createReader(path,
+ OrcFile.readerOptions(conf).filesystem(fs))) {
+
+ SearchArgument sarg = SearchArgumentFactory.newBuilder()
+ .startNot()
+ .lessThan("x", PredicateLeaf.Type.LONG, 100000L)
Review Comment:
The test column data is [0-1023]. I think we should use a no-intersection condition to verify that the pickup is skipped.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1099642316
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,37 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ Path[] paths = new Path[] {
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexCPlusPlus.orc"),
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexJava.orc"),
+ };
+ for (Path path: paths) {
+ FileSystem fs = path.getFileSystem(conf);
+ try (ReaderImpl reader = (ReaderImpl) OrcFile.createReader(path,
+ OrcFile.readerOptions(conf).filesystem(fs))) {
+
+ SearchArgument sarg = SearchArgumentFactory.newBuilder()
+ .startNot()
+ .lessThan("x", PredicateLeaf.Type.LONG, 100000L)
Review Comment:
Thanks @guiyanakuang , done.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on pull request #1387: ORC-1362:Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1420540027
CC @dongjoon-hyun @guiyanakuang @pavibhai
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437730686
I took a glance and found we may have a similar issue on the C++ side: https://github.com/apache/orc/blob/main/c%2B%2B/src/Writer.cc#L422
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] guiyanakuang commented on a diff in pull request #1387: ORC-1362:Enable ENABLE_INDEXES
Posted by "guiyanakuang (via GitHub)" <gi...@apache.org>.
guiyanakuang commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1099615911
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
So, based on this pr, it is not possible to write buildIndex = false and rowIndexStride > 0 , right?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1103015288
##########
java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java:
##########
@@ -1290,7 +1290,12 @@ private void clearStreams() {
private void readStripe() throws IOException {
StripeInformation stripe = beginReadStripe();
planner.parseStripe(stripe, fileIncluded);
- includedRowGroups = pickRowGroups();
+ try {
+ includedRowGroups = pickRowGroups();
+ } catch (Throwable e) {
Review Comment:
Let me give a split answer on this:
* When the inconsistent issue happens should we continue reading? Yes as this is well-defined on what the issue is i.e. row index stride is set but indexes have not been written.
* Should other issues in reading indexes be intercepted? The general answer here is no, as we don't know what the issue is that you are encountering so we don't know if it is correct to intercept it.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100943166
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
Now we have two options to support the configuration:
1. Throw an exception directly when the configuration mismatch.
2. Help users configure `orc.row.index.stride=0` when `orc.create.index=false` is configured,
Using the first scheme will allow users to strongly perceive the orc.row.index.stride parameter, but it will increase the difficulty of use. Users need to manually set the orc.row.index.stride parameter.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1435388491
@deshanxiao thanks for the changes. I will get back to you soon.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1108196204
##########
java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java:
##########
@@ -1290,7 +1290,12 @@ private void clearStreams() {
private void readStripe() throws IOException {
StripeInformation stripe = beginReadStripe();
planner.parseStripe(stripe, fileIncluded);
- includedRowGroups = pickRowGroups();
+ try {
+ includedRowGroups = pickRowGroups();
+ } catch (Throwable e) {
Review Comment:
OK, thank you @pavibhai.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437877579
After a deep look, I can confirm that the C++ side does not have the issue.
The C++ writer only enables writing index with a positive rowIndexStride: https://github.com/apache/orc/blob/main/c%2B%2B/src/Writer.cc#L127
The C++ reader only creates SargsApplier with a positive rowIndexStride in the footer: https://github.com/apache/orc/blob/main/c%2B%2B/src/Reader.cc#L304
@dongjoon-hyun
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100940523
##########
java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java:
##########
@@ -1290,7 +1290,12 @@ private void clearStreams() {
private void readStripe() throws IOException {
StripeInformation stripe = beginReadStripe();
planner.parseStripe(stripe, fileIncluded);
- includedRowGroups = pickRowGroups();
+ try {
+ includedRowGroups = pickRowGroups();
+ } catch (Throwable e) {
Review Comment:
Thank you @pavibhai
My question is when `pickRowGroups `fails (not only the inconsistencies issue) should we continue to read the file ? Or throw an exception directly?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100941130
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
@pavibhai Good suggesions, thank you, let me try.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1432932122
Hi @pavibhai Sorry for my late update. I have modified all comments you mention. Could you please take a look?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1103017157
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
My preference would be for automatic determination so 2.
* if `orc.row.index.stride = 0` then set `orc.create.index=false`
* if `orc.row.index.stride > 0` then set `orc.create.index=true`
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1111410637
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,41 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ // We use ORC files in two languages to test, the previous Java version could not work
+ // well when stripeSize > 0 and orc.create.index=false, now it can skip these row groups.
+ Path[] paths = new Path[] {
+ // Writen by C++ API with schema struct<x:int,y:string> stripeSize=0
Review Comment:
Thank you @pavibhai Fixed it.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on pull request #1387: ORC-1362:Enable ENABLE_INDEXES [Draft]
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1408351626
Work on more test case.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1421978684
> Could you use a different title, @deshanxiao ? This PR only considers Java side, doesn't it?
Done
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437879606
@wgtmac Thank you for the confirmation! Then, we are good.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1099635281
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
Yes, you are right.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437753588
> Thank you for taking a look at it. Do you think we can have consistent behavior in both C++ and Java languages in Apache ORC 1.9?
Yes, I will fix it later this week.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437752641
Thank you for taking a look at it. Do you think we can have consistent behavior in both C++ and Java languages in Apache ORC 1.9?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437885603
Thank you very much! @pavibhai @dongjoon-hyun @guiyanakuang @wgtmac
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362:Enable ENABLE_INDEXES [Draft]
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1098361027
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
Or we could set the rowIndexStride=0 directly.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100240522
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
I was looking at the WriterOptions. Was thinking if we should integrate if there as well. There seem to be 2 places of interest right now.
Constructor. We currently have
```java
buildIndex = OrcConf.ENABLE_INDEXES.getBoolean(tableProperties, conf);
rowIndexStrideValue =
(int) OrcConf.ROW_INDEX_STRIDE.getLong(tableProperties, conf);
```
We should set rowIndexStrideValue to 0 if `buildIndex` is false
Setter for rowIndexStride (I don't see a setter for buildIndex)
```java
public WriterOptions rowIndexStride(int value) {
rowIndexStrideValue = value;
return this;
}
```
Adapt this to say if rowIndexStride is 0 then set buildIndex false, else if rowIndexStride > 0 set buildIndex true.
This way the options will be internally consistent.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100240522
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
I was looking at the WriterOptions. Was thinking if we should integrate if there as well. There seem to be 2 places of interest right now.
Constructor. We currently have
```java
buildIndex = OrcConf.ENABLE_INDEXES.getBoolean(tableProperties, conf);
rowIndexStrideValue =
(int) OrcConf.ROW_INDEX_STRIDE.getLong(tableProperties, conf);
```
We should set rowIndexStrideValue to 0 if `buildIndex` is false
Setter for rowIndexStride (I don't see a setter for buildIndex)
```java
public WriterOptions rowIndexStride(int value) {
rowIndexStrideValue = value;
return this;
}
```
Adapt this to say if rowIndexStride is 0 then set buildIndex false, else if rowIndexStride > 0 set buildIndex true.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1100264665
##########
java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java:
##########
@@ -1290,7 +1290,12 @@ private void clearStreams() {
private void readStripe() throws IOException {
StripeInformation stripe = beginReadStripe();
planner.parseStripe(stripe, fileIncluded);
- includedRowGroups = pickRowGroups();
+ try {
+ includedRowGroups = pickRowGroups();
+ } catch (Throwable e) {
Review Comment:
this feels too broad to swallow any throwable. If the aim is to just deal with inconsistencies of not seeing an index stride and not seeing indexes.
I think a better change is inside the method `pickRowGroups` where after reading rowIndexes we test if row indexes are absent and treat that case as no row groups to skip.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1099642540
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,37 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ Path[] paths = new Path[] {
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexCPlusPlus.orc"),
+ new Path(workDir, "TestOrcFile.testSargSkipPickupGroupWithoutIndexJava.orc"),
Review Comment:
Good catch! Done. Thanks @guiyanakuang
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] deshanxiao commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "deshanxiao (via GitHub)" <gi...@apache.org>.
deshanxiao commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1108278889
##########
java/core/src/java/org/apache/orc/impl/WriterImpl.java:
##########
@@ -189,8 +189,12 @@ public WriterImpl(FileSystem fs,
this.rowIndexStride = 0;
}
- // ORC-1343: We ignore `opts.isBuildIndex` due to the lack of reader support
- this.buildIndex = rowIndexStride > 0;
+ if (!opts.isBuildIndex() && rowIndexStride > 0) {
+ throw new IllegalArgumentException("Please set rowIndexStride to zero " +
Review Comment:
Thank you . Updated it.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a diff in pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on code in PR #1387:
URL: https://github.com/apache/orc/pull/1387#discussion_r1110635665
##########
java/core/src/test/org/apache/orc/impl/TestReaderImpl.java:
##########
@@ -527,4 +527,41 @@ public void testWithoutCompressionBlockSize() throws IOException {
}
}
}
+
+ @Test
+ public void testSargSkipPickupGroupWithoutIndex() throws IOException {
+ Configuration conf = new Configuration();
+ // We use ORC files in two languages to test, the previous Java version could not work
+ // well when stripeSize > 0 and orc.create.index=false, now it can skip these row groups.
+ Path[] paths = new Path[] {
+ // Writen by C++ API with schema struct<x:int,y:string> stripeSize=0
Review Comment:
.nit do you mean row index size?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437671631
Additionally, cc @williamhyun
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #1387: ORC-1362: [Java] Enable ENABLE_INDEXES
Posted by "pavibhai (via GitHub)" <gi...@apache.org>.
pavibhai commented on PR #1387:
URL: https://github.com/apache/orc/pull/1387#issuecomment-1437411088
Thanks for the changes @deshanxiao.
While merging I felt that we should update the description of the PR. I have updated the description of the PR please check to see if this is acceptable.
cc @guiyanakuang @dongjoon-hyun
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org