----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29592/#review66751 -----------------------------------------------------------
It would be helpful for me if you described the basic outline of how these operators fit together in a few paragraphs. The API looks very well thought out and intentional, but I'm having a bit of trouble building a proper mental map of how everything fits together. It seems that there's no concept of a schema in this model. Tuples have fields of String -> object, and that's all. Is this a correct understanding? If so, I wonder if there are cases where we would require knowledge of the data types in the Tuples? Nit: @author tags should go away. Nit: licenses missing on everything. samza-sql/src/main/java/org/apache/samza/sql/api/data/RelationStore.java <https://reviews.apache.org/r/29592/#comment110367> If I understand correctly, this is basically a relation manager that allows you to get any relation based on its spec. Is that correct? samza-sql/src/main/java/org/apache/samza/sql/api/data/RelationStore.java <https://reviews.apache.org/r/29592/#comment110368> Can we retrieve relations by name rather than spec? Seems a little cleaner. samza-sql/src/main/java/org/apache/samza/sql/api/data/RelationStore.java <https://reviews.apache.org/r/29592/#comment110369> It seems liek window state and relations are somewhat unrelated. This class (RelationStore) feels like a generic "Manager" interface that's responsible for holding on to a bunch of stuff to make it easy to access throught the rest of the API. More of a Context object. samza-sql/src/main/java/org/apache/samza/sql/api/data/StreamSpec.java <https://reviews.apache.org/r/29592/#comment110365> What's the idea behind this class. It feels like a place holder for something important, but I'm not quite sure what. samza-sql/src/main/java/org/apache/samza/sql/api/data/StreamSpec.java <https://reviews.apache.org/r/29592/#comment110364> Is this the tuple name, or stream name? Does each tuple have its own name? samza-sql/src/main/java/org/apache/samza/sql/api/data/Tuple.java <https://reviews.apache.org/r/29592/#comment110361> Do we need a corresponding getKey call? samza-sql/src/main/java/org/apache/samza/sql/api/data/Tuple.java <https://reviews.apache.org/r/29592/#comment110362> How do nested data structures work? Will getField return another Tuple, or a Map? samza-sql/src/main/java/org/apache/samza/sql/api/operators/Operator.java <https://reviews.apache.org/r/29592/#comment110370> Is there ever a case where an operator might need multiple stores? samza-sql/src/main/java/org/apache/samza/sql/api/operators/RelationRelationOperator.java <https://reviews.apache.org/r/29592/#comment110372> I'm not crazy about setter methods, in general. I prefer keeping classes immutable, if possible. How often are these setNextOp methods getting called? Who calls them? samza-sql/src/test/java/org/apache/samza/sql/task/StreamSqlTask.java <https://reviews.apache.org/r/29592/#comment110373> A nice description of what's going on here would be pretty useful. This seems like the meat of the example. I can tell that you create a bunch of specs, use the specs to get operators, init the operators, and then wire them together. samza-sql/src/test/java/org/apache/samza/sql/task/StreamSqlTask.java <https://reviews.apache.org/r/29592/#comment110374> It's unclear to me where the input/output are. It seems like things are wired in reverse (i.e. input goes to join, which goes to the window operators). I would have expected the input to go to the window operators, and the join to operato on both windowed relations. - Chris Riccomini On Jan. 5, 2015, 10:19 p.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29592/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2015, 10:19 p.m.) > > > Review request for samza, Chris Riccomini, Navina Ramesh, and Naveen > Somasundaram. > > > Bugs: SAMZA-482 > https://issues.apache.org/jira/browse/SAMZA-482 > > > Repository: samza > > > Description > ------- > > StreamSQL operator API draft > - This is the first draft of the StreamSQL operator APIs > - org.apache.samza.sql.api.* contains definitions of all interface classes > - org.apache.samza.sql.operators.* are skeleton implementation of some > example build-in operators > - src/test/java contains the example application (a stream-join application) > using the above APIs > > > Diffs > ----- > > build.gradle 38383bd9e3f0847d6088a4ea4c1ee6f3dcd1e430 > samza-sql/src/main/java/org/apache/samza/sql/api/data/Relation.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/api/data/RelationSpec.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/api/data/RelationStore.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/api/data/StreamSpec.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/api/data/Tuple.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/api/operators/Operator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/RelationOperator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/RelationRelationOperator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/RelationTupleOperator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/SqlOperatorFactory.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/TupleOperator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/TupleRelationOperator.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/TupleTupleOperator.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/api/operators/spec/RelationOperatorSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/RelationRelationSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/RelationTupleSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/TupleOperatorSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/TupleRelationSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/api/operators/spec/TupleTupleSpec.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/output/SystemStreamOp.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/output/SystemStreamSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/partition/PartitionOp.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/partition/PartitionSpec.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/sql/operators/relation/Join.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/relation/JoinSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/stream/InsertStream.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/stream/InsertStreamSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/window/BoundedTimeWindow.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/window/WindowSpec.java > PRE-CREATION > > samza-sql/src/main/java/org/apache/samza/sql/operators/window/WindowState.java > PRE-CREATION > samza-sql/src/main/java/org/apache/samza/task/SqlTaskContext.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/data/IncomingMessageTuple.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/store/SQLRelationStore.java > PRE-CREATION > samza-sql/src/test/java/org/apache/samza/sql/task/StreamSqlTask.java > PRE-CREATION > settings.gradle 3a01fd66359b8c79954ae8f34eeaf4b2e3fdc0b4 > > Diff: https://reviews.apache.org/r/29592/diff/ > > > Testing > ------- > > > Thanks, > > Yi Pan (Data Infrastructure) > >
