[GitHub] [drill] jnturton commented on a diff in pull request #2583: DRILL-8182: File scan nodes not differentiated by format config

[GitBox <gi...@apache.org>]

jnturton commented on code in PR #2583:
URL: https://github.com/apache/drill/pull/2583#discussion_r914952472


##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/mock/MockTableDef.java:
##########
@@ -84,6 +85,19 @@ public String toString() {
     }
   }
 
+  /**
+   * An unfortunate hack that adds required DrillTableSelection behaviour to
+   * the entries list while keeping its serialised form a JSON array to remain
+   * compatible with existing, serialised logical plan JSON files as may be
+   * found in the Drill unit test code, for example.
+   */
+  public static class MockTableSelection extends ArrayList<MockScanEntry> implements DrillTableSelection {

Review Comment:
   @vvysotskyi this piece has turned out a bit ugly, although it is only in this mock plugin so not very important I suppose. Another possibilty here was to use a wrapper class, rather than extending ArrayList, in order to implement DrillTableSelection but then Jackson serialises the wrapper object and existing logical plan JSON test data is no longer readable. However I don't think there's a lot of such data (perhaps only two files) if it would preferable to modify it. Examples
   
   ```
   ➜  drill git:(8182-excel-data-mixing) ✗ grep -C3 mock **/*.json | grep -C3 selection
   common/src/test/resources/storage_engine_plan.json-      "op" : "scan",
   common/src/test/resources/storage_engine_plan.json-      "@id" : 1,
   common/src/test/resources/storage_engine_plan.json:      "storageengine" : "mock-engine",
   common/src/test/resources/storage_engine_plan.json-      "selection" : null,
   common/src/test/resources/storage_engine_plan.json-      "ref" : null
   common/src/test/resources/storage_engine_plan.json-    }, {
   --
   --
   --
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json-    "op" : "scan",
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json-    "@id" : 1,
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json:    "storageengine" : "mock",
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json-    "selection" : [ {
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json-      "records" : 10,
   exec/java-exec/src/test/resources/functions/conv/conversionTestWithLogicalPlan.json-      "types" : [ {
   --
   --
   exec/java-exec/src/test/resources/scan_screen_logical.json-    "op" : "scan",
   exec/java-exec/src/test/resources/scan_screen_logical.json-    "memo" : "initial_scan",
   exec/java-exec/src/test/resources/scan_screen_logical.json:    "storageengine" : "mock",
   exec/java-exec/src/test/resources/scan_screen_logical.json-    "selection" : [ {
   exec/java-exec/src/test/resources/scan_screen_logical.json-      "records" : 100,
   exec/java-exec/src/test/resources/scan_screen_logical.json-      "types" : [ {
   --
   ```



-- 
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: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org