Till Westmann has posted comments on this change. Change subject: Continue Cleaninig Up File References and Splits ......................................................................
Patch Set 4: (61 comments) https://asterix-gerrit.ics.uci.edu/#/c/1359/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/factory/LocalFSInputStreamFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/factory/LocalFSInputStreamFactory.java: Line 53: protected Pair<String, String>[] inputFileSplits; Seems that a FileSplit would not be too bad here, as it provides a better description with the same footprint .. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/algebricks/algebricks-tests/src/test/java/org/apache/hyracks/algebricks/tests/pushruntime/PushRuntimeTest.java File hyracks-fullstack/algebricks/algebricks-tests/src/test/java/org/apache/hyracks/algebricks/tests/pushruntime/PushRuntimeTest.java: Line 627: public FileSplit createTempFile(NodeControllerService ncs) throws IOException { Not sure I understand the need for temp files here. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/FileSplit.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/FileSplit.java: Line 27: * A node and a managed path inside an IO device. Used to identify a file/dir across the cluster. The comment doesn't seem to be correct, a subclass supports unmanaged files. I think that this class should be abstract with 2 subclasses. To keep the creation of the subclasses reasonably compact, we could have 2 static "create" methods in this class: 1) one that takes a node and a path and creates an UnmanagedFileSplit 2) one that takes a node, a path, and an iodevice and creates an ManagedFileSplit. Line 40: public FileSplit(String node, String path) { Should be private. Line 46: public String toString() { Would be sufficient to implement this in the subclasses. Line 64: public File getFile(IIOManager ioManager) throws HyracksDataException { Would be sufficient to implement this in the subclasses. Line 75: public FileReference getFileRef(IIOManager ioManager) throws HyracksDataException { Would be sufficient to implement this in the subclasses. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IIOManager.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/IIOManager.java: Line 66: * @param relativePath s/relativePath/path/ Line 72: * A file reference based on the mounting point of {@code ioDeviceId} and the passed {@code relativePath} s/relativePath/path/ Line 78: public FileReference getFileRef(String path) throws HyracksDataException; Could we call this "resolvePath" to make it clear that some work is done here? Line 87: public FileReference absToFileRef(String path) throws HyracksDataException; The existence of this method seems to break the IOManager abstraction. We should mark is as deprecated immediately. Could we also call this "resolveAbsPath" to make it clear that some work is done here? https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/MappedFileSplit.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/MappedFileSplit.java: Line 28: public class MappedFileSplit extends FileSplit { Having a "MappedFileSplit" and an "UnmanagedFileSplit" seems inconsistent. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/UnmanagedFileSplit.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/io/UnmanagedFileSplit.java: Line 31: public UnmanagedFileSplit(String node, String path) { empty line .. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java: Line 216: public synchronized FileReference getFile(String path, int ioDevice) throws HyracksDataException { s/getFile/getFileReference/ https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/AbstractFileWriteOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/AbstractFileWriteOperatorDescriptor.java: Line 35: private final IOManager ioManager; s/IOManager/IIOManager/ Line 39: FileWriteOperator(IOManager ioManager, int index) { s/IOManager/IIOManager/ Line 57: writer.close(); Indentation? Line 92: protected abstract IRecordWriter createRecordWriter(IOManager ioManager, FileSplit fileSplit, int index) s/IOManager/IIOManager/ Line 98: return new DeserializedOperatorNodePushable(ctx, new FileWriteOperator((IOManager) ctx.getIOManager(), No cast required. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/LineFileWriteOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/LineFileWriteOperatorDescriptor.java: Line 41: return new FileOutputStream((File) args[0]); indentation. Line 42: } catch (Exception e) { s/Exception/FileNotFoundException|SecurityException/ Line 66: protected IRecordWriter createRecordWriter(IOManager ioManager, FileSplit fileSplit, int index) s/IOManager/IIOManager/ https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/RecordWriter.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/RecordWriter.java: Line 71: if (columns == null) { Indentation. https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/btree/AbstractBTreeOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/btree/AbstractBTreeOperatorTest.java: Line 269: "data/tpch0.002/orders-part2.tbl") }; single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/AbstractRTreeOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/AbstractRTreeOperatorTest.java: Line 217: "data/orders-with-locations-part1.txt") }; Single line Line 328: "data/orders-with-locations-part2.txt") }; Single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexInsertOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexInsertOperatorTest.java: Line 116: createTempFile(nc1) }); Single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexScanOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexScanOperatorTest.java: Line 99: createTempFile(nc1) }); single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexSearchOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexSearchOperatorTest.java: Line 112: createTempFile(nc1) }); single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexStatsOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/am/rtree/RTreeSecondaryIndexStatsOperatorTest.java: Line 64: createTempFile(nc1) }); single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/AbstractIntegrationTest.java: Line 234: String fileName = "f" + aInteger.getAndIncrement() + ".tmp"; Is it necessary to create temp files in the IOManager? If is is, should we have a shared utility for the tests that creates a temp file on an NC? https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/ReplicateOperatorTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/ReplicateOperatorTest.java: Line 111: expectedResultsFileNames[i] = "data" + File.separator + "device0" + File.separator + inputFileName; Seems that we don't need File.separator here if we use '/' in inputFileName ... https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/ScanPrintTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/ScanPrintTest.java: Line 87: "data/tpch0.001/orders.tbl") }; single line Line 126: "data/tpch0.001/orders.tbl") }; single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOptimizedHybridHashJoinTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOptimizedHybridHashJoinTest.java: Line 90: new FileSplit(NC1_ID, path) }); single line? Line 100: "data/tpch0.001/customer4.tbl") }; single line Line 148: "data/tpch0.001/customer3.tbl") }; single line Line 181: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 198: "data/tpch0.001/customer3.tbl") }; single line Line 202: "data/tpch0.001/orders1.tbl") }; single line Line 232: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOrderHashJoinTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOrderHashJoinTest.java: Line 74: "data/tpch0.001/customer.tbl") }; single line Line 83: "data/tpch0.001/orders.tbl") }; single line Line 135: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 153: "data/tpch0.001/customer.tbl") }; single line Line 162: "data/tpch0.001/orders.tbl") }; single line Line 218: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 236: "data/tpch0.001/customer.tbl") }; single line Line 245: "data/tpch0.001/orders.tbl") }; single line Line 301: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 319: "data/tpch0.001/customer.tbl") }; single line Line 328: "data/tpch0.001/orders.tbl") }; single line Line 385: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 411: "data/tpch0.001/orders.tbl") }; single line Line 472: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 490: "data/tpch0.001/customer.tbl") }; single line Line 499: "data/tpch0.001/orders.tbl") }; single line https://asterix-gerrit.ics.uci.edu/#/c/1359/4/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOrderNestedLoopJoinTest.java File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/integration/TPCHCustomerOrderNestedLoopJoinTest.java: Line 128: "data/tpch0.001/customer.tbl") }; single line Line 137: "data/tpch0.001/orders.tbl") }; single line Line 185: IConnectorDescriptor ordJoinConn = new MToNBroadcastConnectorDescriptor(spec); Why is the type of the connector changed here? Line 342: IConnectorDescriptor custJoinConn = new OneToOneConnectorDescriptor(spec); Why is the type of the connector changed here? -- To view, visit https://asterix-gerrit.ics.uci.edu/1359 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I166af8f9b3a2257f94d7b05db94888fb7cb4c79e Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-HasComments: Yes
