dongjoon-hyun commented on a change in pull request #634:
URL: https://github.com/apache/orc/pull/634#discussion_r564170608
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -104,17 +104,10 @@
*/
static int findColumns(SchemaEvolution evolution,
String columnName) {
- try {
- TypeDescription readerColumn =
evolution.getReaderBaseSchema().findSubtype(
- columnName, evolution.isSchemaEvolutionCaseAware);
- TypeDescription fileColumn = evolution.getFileType(readerColumn);
- return fileColumn == null ? -1 : fileColumn.getId();
- } catch (IllegalArgumentException e) {
- if (LOG.isDebugEnabled()){
- LOG.debug("{}", e.getMessage());
- }
- return -1;
- }
+ TypeDescription readerColumn = evolution.getReaderBaseSchema().findSubtype(
+ columnName, evolution.isSchemaEvolutionCaseAware);
+ TypeDescription fileColumn = evolution.getFileType(readerColumn);
+ return fileColumn == null ? -1 : fileColumn.getId();
Review comment:
This PR passed without this change. Shall we have a test coverage for
this change?
And, could you update the function document by adding
```java
@throws IllegalArgumentException ....(description)
```
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -104,17 +104,10 @@
*/
static int findColumns(SchemaEvolution evolution,
String columnName) {
- try {
- TypeDescription readerColumn =
evolution.getReaderBaseSchema().findSubtype(
- columnName, evolution.isSchemaEvolutionCaseAware);
- TypeDescription fileColumn = evolution.getFileType(readerColumn);
- return fileColumn == null ? -1 : fileColumn.getId();
- } catch (IllegalArgumentException e) {
- if (LOG.isDebugEnabled()){
- LOG.debug("{}", e.getMessage());
- }
- return -1;
- }
+ TypeDescription readerColumn = evolution.getReaderBaseSchema().findSubtype(
+ columnName, evolution.isSchemaEvolutionCaseAware);
+ TypeDescription fileColumn = evolution.getFileType(readerColumn);
+ return fileColumn == null ? -1 : fileColumn.getId();
Review comment:
This PR passed without this change. Shall we have a test coverage for
this change?
And, could you update the function document by adding the exception?
```java
@throws IllegalArgumentException ....(description)
```
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -231,11 +224,9 @@ protected RecordReaderImpl(ReaderImpl fileReader,
if (options.getPreFilterColumnNames() != null) {
for (String colName : options.getPreFilterColumnNames()) {
int expandColId = findColumns(evolution, colName);
+ // If the column is not present in the file then this can be ignored
from read.
if (expandColId != -1) {
filterColIds.add(expandColId);
- } else {
- throw new IllegalArgumentException("Filter could not find column
with name: " +
- colName + " on " + evolution.getReaderBaseSchema());
Review comment:
Could you confirm that what is the new error message? I understand that
the thrown exception is the same, `IllegalArgumentException`, but I'm not sure
about the information on the error message.
##########
File path:
java/mapreduce/src/test/org/apache/orc/mapred/TestOrcFileEvolution.java
##########
@@ -312,6 +314,26 @@ public void testPositional2() {
false, addSarg, true);
}
+ private void checkEvolutionPosn(String writerType, String readerType,
Review comment:
This function seems to be duplicated mostly with `checkEvolution`. Is
the only difference is the column name, `_col0` and `a`?
##########
File path:
java/mapreduce/src/test/org/apache/orc/mapred/TestOrcFileEvolution.java
##########
@@ -235,35 +235,37 @@ public void testListEvolution() {
@Test
public void testPreHive4243CheckEqual() {
// Expect success on equal schemas
- checkEvolution("struct<_col0:int,_col1:string>",
+ checkEvolutionPosn("struct<_col0:int,_col1:string>",
"struct<_col0:int,_col1:string>",
struct(1, "foo"),
- struct(1, "foo", null), false, addSarg, false);
+ struct(1, "foo", null),
+ false, addSarg, false);
}
@Test
public void testPreHive4243Check() {
// Expect exception on strict compatibility check
thrown.expectMessage("HIVE-4243");
- checkEvolution("struct<_col0:int,_col1:string>",
+ checkEvolutionPosn("struct<_col0:int,_col1:string>",
"struct<_col0:int,_col1:string,_col2:double>",
struct(1, "foo"),
struct(1, "foo", null), false, addSarg, false);
}
@Test
public void testPreHive4243AddColumn() {
- checkEvolution("struct<_col0:int,_col1:string>",
+ checkEvolutionPosn("struct<_col0:int,_col1:string>",
"struct<_col0:int,_col1:string,_col2:double>",
struct(1, "foo"),
- struct(1, "foo", null), true, addSarg, false);
+ struct(1, "foo", null),
+ true, addSarg, false);
Review comment:
Let's keep the original form to reduce the patch size.
##########
File path:
java/mapreduce/src/test/org/apache/orc/mapred/TestOrcFileEvolution.java
##########
@@ -235,35 +235,37 @@ public void testListEvolution() {
@Test
public void testPreHive4243CheckEqual() {
// Expect success on equal schemas
- checkEvolution("struct<_col0:int,_col1:string>",
+ checkEvolutionPosn("struct<_col0:int,_col1:string>",
"struct<_col0:int,_col1:string>",
struct(1, "foo"),
- struct(1, "foo", null), false, addSarg, false);
+ struct(1, "foo", null),
+ false, addSarg, false);
Review comment:
Let's keep the original form to reduce the patch size.
##########
File path:
java/mapreduce/src/test/org/apache/orc/mapred/TestOrcFileEvolution.java
##########
@@ -312,6 +314,26 @@ public void testPositional2() {
false, addSarg, true);
}
+ private void checkEvolutionPosn(String writerType, String readerType,
Review comment:
Ya. I also don't have a better option. +1 for adding 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.
For queries about this service, please contact Infrastructure at:
[email protected]
