Till Westmann has posted comments on this change. Change subject: Support Change Feeds and Ingestion of Records with MetaData ......................................................................
Patch Set 11: (23 comments) https://asterix-gerrit.ics.uci.edu/#/c/621/11/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 349: } Independent of where these methods will live, they could be a little shorter and clearer by factoring out some common code: public static void prepareMetaKeyAccessExpression(List<String> field, LogicalVariable resVar, List<Mutable<ILogicalExpression>> assignExpressions, List<LogicalVariable> vars, List<Mutable<ILogicalExpression>> varRefs, IVariableContext context) { IFunctionInfo finfoMeta = FunctionUtil.getFunctionInfo(AsterixBuiltinFunctions.META); ScalarFunctionCallExpression f = createFieldAccessExpression(new ScalarFunctionCallExpression(finfoMeta, new MutableObject<ILogicalExpression>(new VariableReferenceExpression(resVar))), field); assignExpressions.add(new MutableObject<ILogicalExpression>(f)); LogicalVariable v = context.newVar(); vars.add(v); if (varRefs != null) { varRefs.add(new MutableObject<ILogicalExpression>(new VariableReferenceExpression(v))); } } public static void prepareVarAndExpression(List<String> field, LogicalVariable resVar, List<LogicalVariable> vars, List<Mutable<ILogicalExpression>> assignExpressions, List<Mutable<ILogicalExpression>> varRefs, IVariableContext context) { ScalarFunctionCallExpression f = createFieldAccessExpression(new VariableReferenceExpression(DUMMY_VAR), field); f.substituteVar(DUMMY_VAR, resVar); assignExpressions.add(new MutableObject<ILogicalExpression>(f)); LogicalVariable v = context.newVar(); vars.add(v); if (varRefs != null) { varRefs.add(new MutableObject<ILogicalExpression>(new VariableReferenceExpression(v))); } } private static ScalarFunctionCallExpression createFieldAccessExpression(ILogicalExpression target, List<String> field) { FunctionIdentifier functionIdentifier; IAObject value; if (field.size() > 1) { functionIdentifier = AsterixBuiltinFunctions.FIELD_ACCESS_NESTED; value = new AOrderedList(field); } else { functionIdentifier = AsterixBuiltinFunctions.FIELD_ACCESS_BY_NAME; value = new AString(field.get(0)); } IFunctionInfo finfoAccess = FunctionUtil.getFunctionInfo(functionIdentifier); return new ScalarFunctionCallExpression(finfoAccess, new MutableObject<>(target), new MutableObject<>(new ConstantExpression(new AsterixConstantValue(value)))); } https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java File asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java: Line 409: public String getFeedName() { Seems to be unused. Line 418: public FeedConnectionRequest getConnectionRequest() { Seems to be unused. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 196: "Unable to load dataset " + clffs.getDatasetName() + " since it is a read only dataset"); > "read only" is not entirely correct. I think that this is related to another discussion we had. The point of that discussion was that a dataset that is the target of a change feed should not be modifiable by other operations than the change feed and so it would be read-only. So it seems that we might be mixing the concepts of "target of a feed dataset" and "hasMeta" here, as both only appear together right now. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java File asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java: Line 49: private static Logger LOGGER = Logger.getLogger(AsterixPropertiesAccessor.class.getName()); Revert file? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/api/IStreamDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IStreamDataParser.java: Line 56: public default void appendKeys(final ArrayTupleBuilder tb) throws IOException { Seems unused, can we remove it? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java: Line 55: public CounterTimerTupleForwarder(int batchSize, long batchInterval) { Make this one private, if we have a factory method? Line 149: public static CounterTimerTupleForwarder create(Map<String, String> configuration) { Move the factory method closer to the constructor? (Or put this code into the constructor). https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/RateControlledTupleForwarder.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/RateControlledTupleForwarder.java: Line 43: public RateControlledTupleForwarder(long interTupleInterval) { Same comments as for CounterTimerTupleForwarder. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java File asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java: Line 38: private final static Logger LOGGER = Logger.getLogger(FrameDistributor.class.getName()); revert file? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketClientInputStreamProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketClientInputStreamProvider.java: Line 96: public void setFeedLogManager(FeedLogManager feedLogManager) { This is apparently never used. Will we need it in the future? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataUtils.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataUtils.java: Line 127: throw new AsterixException(e); Some additional message here would be nice ... Line 233: if (libraryAndFactory[0].contains(".")) { if (! ...) throw, after that declare and initialize variables. Line 244: throw new AsterixException(e); Some additional message here would be nice ... Line 259: throw new AsterixException(e); Some additional message here would be nice ... https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/util/TweetGenerator.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/TweetGenerator.java: Line 35: private static final Logger LOGGER = Logger.getLogger(TweetGenerator.class.getName()); revert file? https://asterix-gerrit.ics.uci.edu/#/c/621/11/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 } Why did this format change? https://asterix-gerrit.ics.uci.edu/#/c/621/11/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 386: FeedCollectOperatorDescriptor feedCollector = null; Could also be declared where it's written. Line 391: RecordDescriptor feedDesc; Move declaration to initialization? Could even make it final :) Line 401: ISerializerDeserializer serde = AqlSerializerDeserializerProvider.INSTANCE inline "serde"? Line 407: FeedPolicyEntity feedPolicy = (FeedPolicyEntity) (feedDataSource).getProperties() Why do we need parens around "feedDataSource"? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/FeedDataSource.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/FeedDataSource.java: Line 134: public List<Integer> getKeySourceIndicator() { This method seems to be unused. https://asterix-gerrit.ics.uci.edu/#/c/621/11/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 228: } catch (Exception e) { 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: 11 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
