abdullah alamoudi has posted comments on this change. Change subject: Support Change Feeds and Ingestion of Records with MetaData ......................................................................
Patch Set 9: (36 comments) https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FunctionToVariableRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/FunctionToVariableRule.java: Line 50: // The rule can only apply once. > Why the name of the rule needs to be changed? I changed the name because I also use this rule to replace PK field-access in change feeds which don't have a meta part. See test case "change-feed". I don't mind changing the name back though. just thought it wasn't really representative of what the rule does. I can change it back or maybe make it: MetaAndChangeFeedFunctionToVariableRule? Line 54: @Override > We can include some variable definition tracing (recursively) in this rewri Got it 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.getDataRecordVariabl Done 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.getDataRecordVariabl Done 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 ... Unless you're debugging and wants to have a break point next to the exception :D Done Line 122: for (int i = 0; i < (n / 2); i++) { > remove auto-parenthesis? Done 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/? Done Line 263: if (key.size() > 1) { > It seems that the only differences between the 2 branches are: Done 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? Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-meta-pk-in-meta/feed-with-meta-pk-in-meta.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-meta-pk-in-meta/feed-with-meta-pk-in-meta.1.ddl.aql: Line 45: ("reader"="localfs"), > tab -> spaces Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-meta-pk-in-meta/feed-with-meta-pk-in-meta.3.sleep.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-meta-pk-in-meta/feed-with-meta-pk-in-meta.3.sleep.aql: Line 24: 4000 > what does this query test? This just sleeps for 4 seconds to give a chance for the feed to completely disconnect and close the dataset. we have a feed bug that causes the feed job to come back before all the records are committed. This is very hacky but I'd rather have it than not for now. Will remove it once it is made completely deterministic https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_02/feeds_02.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_02/feeds_02.1.ddl.aql: Line 36: timestamp : string > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_10/feeds_10.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_10/feeds_10.1.ddl.aql: Line 39: } > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_11/feeds_11.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_11/feeds_11.1.ddl.aql: Line 36: } > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_12/feeds_12.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_12/feeds_12.1.ddl.aql: Line 39: } > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/feeds/issue_230_feeds/issue_230_feeds.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/issue_230_feeds/issue_230_feeds.1.ddl.aql: Line 36: } > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-app/src/test/resources/runtimets/queries/hints/issue_251_dataset_hint_7/issue_251_dataset_hint_7.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/hints/issue_251_dataset_hint_7/issue_251_dataset_hint_7.1.ddl.aql: Line 36: } > WS Done 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 ... Done 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) co Done 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 st Agreed and I think many interfaces in External data land needs to be CAREFULLY looked at. hopefully soon. Line 90: th.printStackTrace(); > This is temporary? Done 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 Done 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? Done Line 63: System.err.println("operation on " + resourecePath.getFile().getAbsolutePath() + " Succeded"); > Remove or log this? Done 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? I believe so. The only change here is moving the configure() call and perform configuration in the constructor. I am not sure why Gerrit is showing large blocks of changes. 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? Done Line 110: th.printStackTrace(); > Do we keep this? Done Line 119: th.printStackTrace(); > Do we keep this? Done 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? Done 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 ? Done 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? this was not caught before Yingyi's change to also compare what is after the time field. The problem was that this test was not run on Jenkins (We need to check which tests are running and which are not!). https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java: Line 804: keyType = recType.getSubFieldType(pidxKeyFieldNames.get(j)); > IAType keyType = recType.getSubFieldType(pidxKeyFieldNames.get(j)); Done 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 f Done https://asterix-gerrit.ics.uci.edu/#/c/621/9/asterix-om/src/main/java/org/apache/asterix/om/types/ARecordType.java File asterix-om/src/main/java/org/apache/asterix/om/types/ARecordType.java: Line 306: public void getFieldTypes(List<List<String>> fields, List<IAType> emptyList) throws AlgebricksException { > void getFieldTypes(List<List<String>> fields, List<IAType> emptyList) Done 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? yes, it is the position of the record variable == the size of pk. Line 229: } > remove this try-catch? Done -- 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
