Copilot commented on code in PR #61391:
URL: https://github.com/apache/doris/pull/61391#discussion_r2939827774
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java:
##########
@@ -384,6 +392,64 @@ public void testValidateIncrementalReadParams() throws
UserException {
}
}
+ @Test
+ public void testPaimonDataSystemTableForceJniEvenWhenNativeSupported()
throws UserException {
+ TupleDescriptor desc = new TupleDescriptor(new TupleId(3));
+ PaimonScanNode paimonScanNode = new PaimonScanNode(new PlanNodeId(1),
desc, false, sv);
Review Comment:
`PaimonScanNode` in this branch only has a constructor that takes a
`ScanContext` (see `PaimonScanNode(PlanNodeId, TupleDescriptor, boolean,
SessionVariable, ScanContext)`). This test uses a 4-arg constructor, which will
not compile; please pass `ScanContext.EMPTY` like the other tests in this class.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java:
##########
@@ -384,6 +392,64 @@ public void testValidateIncrementalReadParams() throws
UserException {
}
}
+ @Test
+ public void testPaimonDataSystemTableForceJniEvenWhenNativeSupported()
throws UserException {
+ TupleDescriptor desc = new TupleDescriptor(new TupleId(3));
+ PaimonScanNode paimonScanNode = new PaimonScanNode(new PlanNodeId(1),
desc, false, sv);
+ PaimonScanNode spyPaimonScanNode = Mockito.spy(paimonScanNode);
+
+ DataFileMeta dfm = DataFileMeta.forAppend("f1.parquet", 64L * 1024 *
1024, 1L, SimpleStats.EMPTY_STATS,
+ 1L, 1L, 1L, Collections.<String>emptyList(), null,
FileSource.APPEND,
+ Collections.<String>emptyList(), null, null,
Collections.<String>emptyList());
+ BinaryRow binaryRow = BinaryRow.singleColumn(1);
+ DataSplit dataSplit = DataSplit.builder()
+ .rawConvertible(true)
+ .withPartition(binaryRow)
+ .withBucket(1)
+ .withBucketPath("file://b1")
+ .withDataFiles(Collections.singletonList(dfm))
+ .build();
+
+
Mockito.doReturn(Collections.singletonList(dataSplit)).when(spyPaimonScanNode).getPaimonSplitFromAPI();
+ mockNativeReader(spyPaimonScanNode);
+
Review Comment:
This test stubs `supportNativeReader()` to return true
(`mockNativeReader(...)`) while also stubbing `sv.isForceJniScanner()` to
false, so `getSplits()` will follow the native-reader branch and require
`storagePropertiesMap` to be initialized (it isn't, since `doInitialize()`
isn't called). Either inject `storagePropertiesMap` like `testSplitWeight()`
does, or adjust stubs so the JNI branch is taken.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java:
##########
@@ -384,6 +392,64 @@ public void testValidateIncrementalReadParams() throws
UserException {
}
}
+ @Test
+ public void testPaimonDataSystemTableForceJniEvenWhenNativeSupported()
throws UserException {
+ TupleDescriptor desc = new TupleDescriptor(new TupleId(3));
+ PaimonScanNode paimonScanNode = new PaimonScanNode(new PlanNodeId(1),
desc, false, sv);
+ PaimonScanNode spyPaimonScanNode = Mockito.spy(paimonScanNode);
+
+ DataFileMeta dfm = DataFileMeta.forAppend("f1.parquet", 64L * 1024 *
1024, 1L, SimpleStats.EMPTY_STATS,
+ 1L, 1L, 1L, Collections.<String>emptyList(), null,
FileSource.APPEND,
+ Collections.<String>emptyList(), null, null,
Collections.<String>emptyList());
+ BinaryRow binaryRow = BinaryRow.singleColumn(1);
+ DataSplit dataSplit = DataSplit.builder()
+ .rawConvertible(true)
+ .withPartition(binaryRow)
+ .withBucket(1)
+ .withBucketPath("file://b1")
+ .withDataFiles(Collections.singletonList(dfm))
+ .build();
+
+
Mockito.doReturn(Collections.singletonList(dataSplit)).when(spyPaimonScanNode).getPaimonSplitFromAPI();
+ mockNativeReader(spyPaimonScanNode);
+
+ PaimonSource source = Mockito.mock(PaimonSource.class);
+ PaimonSysExternalTable binlogTable =
Mockito.mock(PaimonSysExternalTable.class);
+ Mockito.when(binlogTable.getSysTableType()).thenReturn("binlog");
+ Mockito.when(source.getExternalTable()).thenReturn(binlogTable);
+ spyPaimonScanNode.setSource(source);
Review Comment:
`PaimonSysExternalTable` is referenced here but there is no such type in the
current codebase (repo-wide search only finds these test lines). This will fail
compilation; please use the actual system-table type used in FE for Paimon
system tables, or add the missing type as part of the change.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/paimon/source/PaimonScanNodeTest.java:
##########
@@ -384,6 +392,64 @@ public void testValidateIncrementalReadParams() throws
UserException {
}
}
+ @Test
+ public void testPaimonDataSystemTableForceJniEvenWhenNativeSupported()
throws UserException {
+ TupleDescriptor desc = new TupleDescriptor(new TupleId(3));
+ PaimonScanNode paimonScanNode = new PaimonScanNode(new PlanNodeId(1),
desc, false, sv);
+ PaimonScanNode spyPaimonScanNode = Mockito.spy(paimonScanNode);
+
+ DataFileMeta dfm = DataFileMeta.forAppend("f1.parquet", 64L * 1024 *
1024, 1L, SimpleStats.EMPTY_STATS,
+ 1L, 1L, 1L, Collections.<String>emptyList(), null,
FileSource.APPEND,
+ Collections.<String>emptyList(), null, null,
Collections.<String>emptyList());
+ BinaryRow binaryRow = BinaryRow.singleColumn(1);
+ DataSplit dataSplit = DataSplit.builder()
+ .rawConvertible(true)
+ .withPartition(binaryRow)
+ .withBucket(1)
+ .withBucketPath("file://b1")
+ .withDataFiles(Collections.singletonList(dfm))
+ .build();
+
+
Mockito.doReturn(Collections.singletonList(dataSplit)).when(spyPaimonScanNode).getPaimonSplitFromAPI();
+ mockNativeReader(spyPaimonScanNode);
+
+ PaimonSource source = Mockito.mock(PaimonSource.class);
+ PaimonSysExternalTable binlogTable =
Mockito.mock(PaimonSysExternalTable.class);
+ Mockito.when(binlogTable.getSysTableType()).thenReturn("binlog");
+ Mockito.when(source.getExternalTable()).thenReturn(binlogTable);
+ spyPaimonScanNode.setSource(source);
+
+ long maxInitialSplitSize = 32L * 1024L * 1024L;
+ long maxSplitSize = 64L * 1024L * 1024L;
+ FileSplitter fileSplitter = new FileSplitter(maxInitialSplitSize,
maxSplitSize, 0);
+ try {
+ java.lang.reflect.Field field =
FileQueryScanNode.class.getDeclaredField("fileSplitter");
+ field.setAccessible(true);
+ field.set(spyPaimonScanNode, fileSplitter);
+ } catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new RuntimeException("Failed to inject FileSplitter into
PaimonScanNode test", e);
+ }
+
+ Mockito.when(sv.isForceJniScanner()).thenReturn(false);
+ Mockito.when(sv.getIgnoreSplitType()).thenReturn("NONE");
+
Mockito.when(sv.isEnableRuntimeFilterPartitionPrune()).thenReturn(false);
+ Mockito.when(sv.getMaxSplitSize()).thenReturn(maxSplitSize);
+
+ Assert.assertTrue(spyPaimonScanNode.shouldForceJniForSystemTable());
+ List<org.apache.doris.spi.Split> splits =
spyPaimonScanNode.getSplits(1);
Review Comment:
`shouldForceJniForSystemTable()` is invoked here, but there is no such
method on `PaimonScanNode` (or any superclass) in this branch (repo-wide search
returns no definition). The test will not compile; please update the test to
call an existing API, or add the intended method/behavior in production code as
part of this PR.
##########
fe/pom.xml:
##########
@@ -221,7 +221,7 @@ under the License.
<module>be-java-extensions</module>
</modules>
<properties>
-
<doris.hive.catalog.shade.version>3.1.0</doris.hive.catalog.shade.version>
+
<doris.hive.catalog.shade.version>3.1.1</doris.hive.catalog.shade.version>
<!-- iceberg 1.9.1 depends avro on 1.12 -->
Review Comment:
The comment above `avro.version` still references Iceberg 1.9.1, but this PR
bumps `iceberg.version` to 1.10.1. Please update the comment to match the new
Iceberg version (or make it version-agnostic) to avoid future confusion when
maintaining these coupled versions.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
