Till Westmann has posted comments on this change. Change subject: Support Change Feeds and Ingestion of Records with MetaData ......................................................................
Patch Set 9: (23 comments) It would also be nice, if we could have the big test files only once. Can we access asterix-app/data in the tests? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceDynamicTypeCastRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceDynamicTypeCastRule.java: Line 115: } Could we unify this with the code around AqlDataSource.getDataRecordVariable ? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceStaticTypeCastForInsertRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceStaticTypeCastForInsertRule.java: Line 106: } Could we unify this with the code around AqlDataSource.getDataRecordVariable ? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetClosedRecordConstructorsRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SetClosedRecordConstructorsRule.java: Line 78: } catch (Exception e) { This try-catch seems useless ... https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UnnestToDataScanRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UnnestToDataScanRule.java: Line 160: throw new AlgebricksException("No positional variables are allowed over datasets."); s/datasets/feeds/? Line 263: if (key.size() > 1) { It seems that the only differences between the 2 branches are: FIELD_ACCESS_NESTED vs FIELD_ACCESS_BY_NAME and new AOrderedList(key) vs new AString(key.get(0)) If so, can we remove some of the duplication? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 494: if (field.size() > 1) { Is this the same code as in UnnestToDataScanRule? If so, could we pull that out? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/api/IAdapterRuntimeManager.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IAdapterRuntimeManager.java: Line 66: * @return the instance of the feed adapter (an implementation of {@code IFeedAdapter}) in use. Comment and signature are inconsistent ... https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java: Line 39: public void reset(IRecordReader<?> reader) throws HyracksDataException; Might be better to make those IOExceptions and handle those at the (few) consumption points. https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java: Line 73: addPrimaryKeys(tb, record); We don't need to fix this now, but I'm not convinced that this interface style works well. It seems that we need to look at the record multiple times. Instead I would prefer to look at the record once and write the result to multiple places (unless that requires additional copies - so it might well be that that's not feasible :) ). Line 90: th.printStackTrace(); This is temporary? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java: Line 141: * WS https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesCommitOperatorDescriptor.java File asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalDatasetIndexesCommitOperatorDescriptor.java: Line 55: System.err.println("performing the operation on " + resourecePath.getFile().getAbsolutePath()); Remove or log this? Line 63: System.err.println("operation on " + resourecePath.getFile().getAbsolutePath() + " Succeded"); Remove or log this? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/parser/DelimitedDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/DelimitedDataParser.java: Line 103: @Override Are these methods unchanged? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/parser/RecordWithMetadataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/RecordWithMetadataParser.java: Line 87: th.printStackTrace(); Do we keep this? Line 110: th.printStackTrace(); Do we keep this? Line 119: th.printStackTrace(); Do we keep this? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/parser/RecordWithPKDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/RecordWithPKDataParser.java: Line 48: throw new HyracksDataException(e); Why convert this, if the signature allows an IOException? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java: Line 126: } catch (Exception e) { AsterixException|IOException ? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-installer/src/test/resources/integrationts/lifecycle/results/asterix-lifecycle/backupRestore/backupRestore.1.adm File asterix-installer/src/test/resources/integrationts/lifecycle/results/asterix-lifecycle/backupRestore/backupRestore.1.adm: Line 1: { "DataverseName": "backupDataverse", "DataFormat": "org.apache.asterix.runtime.formats.NonTaggedDataFormat", "Timestamp": "Wed Apr 24 16:13:46 PDT 2013", "PendingOp": 0i32 } That change seem strange - what causes this? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java: Line 603: public static ARecordType getOutputType(IFeed feed, Map<String, String> configuration) throws AlgebricksException { It seems that this method is nearly identical to the previous one. Can we factor the common parts out? https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java File asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java: Line 68: private final int numOfPrimaryKeys; This is a position, not a count, right? Line 229: } remove this try-catch? -- To view, visit https://asterix-gerrit.ics.uci.edu/621 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If136a03d424970132dfb09f0dda56e160d4c0078 Gerrit-PatchSet: 9 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
