----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33142/#review79941 -----------------------------------------------------------
I will continue the rest of the review after this patch is rebased and front-end code is moved to samza-sql-calcite. One high-level comment: adding more complete Java doc, comments/annotations in the code would help in this early stage. samza-sql/src/main/java/org/apache/samza/sql/data/IntermediateMessageTuple.java <https://reviews.apache.org/r/33142/#comment129620> nit: Java doc. samza-sql/src/main/java/org/apache/samza/sql/expressions/Expression.java <https://reviews.apache.org/r/33142/#comment129622> Not used? samza-sql/src/main/java/org/apache/samza/sql/expressions/Expression.java <https://reviews.apache.org/r/33142/#comment129621> nit: Java doc missing. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129625> What's the relationship between RelNode and RexNode here? Some description would be helpful. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129624> Java doc samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129628> It would be nicer if more comments/annotation is added to explain the code here. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129627> nit: In Samza code convention, we don't use suffix '_' to variable names. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129632> Just a question, is it supposed to throw exception here even in a real implementation? Or this is just a place-holder that would need to be implemented later? samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129638> nit: no suffix '_' in variable names. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129650> It would be easier to read if the code section to create the second BlockStatement can be folded into another method. samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129653> Is "Buzz" the intended real class name or is this just for experiment? Wouldn't it be better to be "SqlExpression"? samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java <https://reviews.apache.org/r/33142/#comment129654> Shouldn't this be expr.implemented? samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaUtils.java <https://reviews.apache.org/r/33142/#comment129663> Same here. It would be nicer to add Java doc here to explain what this does. samza-sql/src/main/java/org/apache/samza/sql/metadata/RelDataTypeToAvroSchemaConverter.java <https://reviews.apache.org/r/33142/#comment129666> One question here: it seems that this converter should be an abstract class that can be extended to convert RelDataType to Avro or JSON based on implementation classes? samza-sql/src/main/java/org/apache/samza/sql/metadata/Stream.java <https://reviews.apache.org/r/33142/#comment129669> What's the meaning of this parent? samza-sql/src/main/java/org/apache/samza/sql/metadata/Stream.java <https://reviews.apache.org/r/33142/#comment129670> I am not sure whether I fully understand this interface method: from Calcite code base, it seems that this should return an iterator for the stream? This seems to be a polling method to get the rows from the stream. However, Samza tasks are not polling the streams to get the messages. Instead, Samza SystemConsumers delivers (i.e. pushes) the messages from the physical streams to Samza tasks, and hence, the physical operators. How is this stream() API used in Calcite? Can we leave it un-implemented? samza-sql/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorSpec.java <https://reviews.apache.org/r/33142/#comment129686> For operators that keep states that should survive across restarts, UUID may not work. For example, getId() is used in WindowOperator to identify the state store name, which should be the same across re-starts. samza-sql/src/main/java/org/apache/samza/sql/operators/factory/TypeAwareOperatorSpec.java <https://reviews.apache.org/r/33142/#comment129688> If we keep this in samza-sql-core module, can we remove the reference to Calcite specific class name and replace it w/ Schema class here? samza-sql/src/main/java/org/apache/samza/sql/operators/insert/InsertToStreamOp.java <https://reviews.apache.org/r/33142/#comment129689> I think that we may be able to combine these two operators. Let me think about it a bit more. samza-sql/src/main/java/org/apache/samza/sql/operators/scan/ProjectableFilterableStreamScanSpec.java <https://reviews.apache.org/r/33142/#comment129692> These calcite specific class should be moved out-of samza-sql-core module. - Yi Pan (Data Infrastructure) On April 13, 2015, 9:04 p.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33142/ > ----------------------------------------------------------- > > (Updated April 13, 2015, 9:04 p.m.) > > > Review request for samza and Milinda Pathirage. > > > Bugs: SAMZA-561 > https://issues.apache.org/jira/browse/SAMZA-561 > > > Repository: samza > > > Description > ------- > > [SAMZA-561] Review in progress > > Post Milinda's patch for SAMZA-561 to ease the comment and discussion. > > > Diffs > ----- > > build.gradle 97de3a28f6379e3862eec845da87587b1d4f742e > gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e > samza-sql/src/main/java/org/apache/samza/sql/Utils.java PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/OperatorSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/data/IntermediateMessageTuple.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/data/serializers/SqlAvroSerdeFactory.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/expressions/Expression.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaCompiler.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/expressions/RexToJavaUtils.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/metadata/AvroSchemaConverter.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/metadata/RelDataTypeToAvroSchemaConverter.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/metadata/Stream.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorFactoryImpl.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/factory/TypeAwareOperatorSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/insert/InsertToStreamOp.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/insert/InsertToStreamSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/project/ProjectOp.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/project/ProjectSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/scan/ProjectableFilterableStreamScanOp.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/scan/ProjectableFilterableStreamScanSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/scan/StreamScanSpec.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/planner/ExecutionPlanner.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/planner/QueryPlanner.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/planner/rules/FilterableStreamScanRule.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/planner/rules/ProjectableStreamScanRule.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/planner/rules/RemoveIdentityProjectRule.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/rel/ProjectableFilterableStreamScan.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/rel/StreamScan.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/task/sql/StreamSqlTask.java > PRE-CREATION > > samza-sql/src/test/java/org/apache/samza/sql/data/serializers/SqlAvroSerdeTest.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/planner/QueryPlannerTest.java > PRE-CREATION > > samza-sql/src/test/java/org/apache/samza/sql/planner/SamzaStreamTableFactory.java > PRE-CREATION > > samza-sql/src/test/java/org/apache/samza/sql/planner/TestExecutionPlanner.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/planner/TestQueryPlanner.java > PRE-CREATION > > samza-sql/src/test/java/org/apache/samza/sql/planner/TestRexToJavaCompiler.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/test/Constants.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/test/Utils.java PRE-CREATION > > samza-sql/src/test/java/org/apache/samza/sql/test/metadata/TestAvroSchemaConverter.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/task/sql/RandomOperatorTask.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/task/sql/StreamSqlTask.java > PRE-CREATION > samza-sql/src/test/resources/orders.avsc PRE-CREATION > samza-sql/src/test/resources/orders.json PRE-CREATION > samza-test/src/main/config/sql-filter.properties PRE-CREATION > > samza-test/src/main/java/org/apache/samza/test/integration/sql/OrdersStreamFactory.java > PRE-CREATION > samza-test/src/main/java/org/apache/samza/test/integration/sql/SqlTask.java > PRE-CREATION > samza-test/src/main/python/integration_tests.py > df64e239a2e467c8e4429dbeb7039f1aa9965ecc > samza-test/src/main/python/requirements.txt > 2ae95908248516b5b26e671f24fa680f7b801675 > samza-test/src/main/python/samza_job_yarn_deployer.py > 38635ca5899c43fb61d6b4042e8543f0508fd41b > samza-test/src/main/python/tests/sql_tests.py PRE-CREATION > samza-test/src/main/resources/orders.avsc PRE-CREATION > samza-test/src/main/resources/orders.json PRE-CREATION > > Diff: https://reviews.apache.org/r/33142/diff/ > > > Testing > ------- > > > Thanks, > > Yi Pan (Data Infrastructure) > >
