Refactoring to use schema type for comparison. #614
Conversation
| @@ -137,13 +137,13 @@ private Schema getKsqlType(final String sqlType) { | |||
| } | |||
|
|
|||
| private String getKsqlTypeInJson(final Schema schemaType) { | |||
| if (schemaType == Schema.INT64_SCHEMA) { | |||
| if (schemaType.type() == Schema.Type.INT64) { | |||
There was a problem hiding this comment.
why not use switch statement here instead a bunch of IFs? To make a code style consistent with what has been done earlier in the PR.
There was a problem hiding this comment.
We have lots of code that is doing switch or if on the schema.type() - this is a bit of a code smell IMO. Can we do this in a more OO way? i.e, maybe a registry or similar where we get an object for a schema.type(), where the object has methods on it like enforceFieldType etc?
| @@ -168,8 +168,8 @@ public void testSimpleAggregateLogicalPlan() throws Exception { | |||
| Assert.assertTrue(aggregateNode.getGroupByExpressions().size() == 1); | |||
| Assert.assertTrue(aggregateNode.getGroupByExpressions().get(0).toString().equalsIgnoreCase("TEST1.COL0")); | |||
| Assert.assertTrue(aggregateNode.getRequiredColumnList().size() == 2); | |||
| Assert.assertTrue(aggregateNode.getSchema().fields().get(1).schema() == Schema.FLOAT64_SCHEMA); | |||
| Assert.assertTrue(aggregateNode.getSchema().fields().get(2).schema() == Schema.INT64_SCHEMA); | |||
| Assert.assertTrue(aggregateNode.getSchema().fields().get(1).schema().type() == Schema.Type.FLOAT64); | |||
There was a problem hiding this comment.
can we change these to use assertThat(..)
| Assert.assertTrue(projectedSchemaKStream.getSchema().field("COL0").schema() == Schema.INT64_SCHEMA); | ||
| Assert.assertTrue(projectedSchemaKStream.getSchema().field("COL2").schema() == Schema.STRING_SCHEMA); | ||
| Assert.assertTrue(projectedSchemaKStream.getSchema().field("COL3").schema() == Schema.FLOAT64_SCHEMA); | ||
| Assert.assertTrue(projectedSchemaKStream.getSchema().field("COL0").schema().type() == Schema.Type.INT64); |
| Assert.assertTrue(exprType0 == Schema.BOOLEAN_SCHEMA); | ||
| Assert.assertTrue(exprType1 == Schema.BOOLEAN_SCHEMA); | ||
| Assert.assertTrue(exprType2 == Schema.BOOLEAN_SCHEMA); | ||
| Assert.assertTrue(exprType0.type() == Schema.Type.BOOLEAN); |
| } else { | ||
| throw new KsqlFunctionException( | ||
| "Invalid cast operation: Cannot cast " + exprStr + " to Integer."); | ||
| switch (schema.type()) { |
There was a problem hiding this comment.
Would it be possible to push down this logic into Schema.TYPE enums? - i.e. implement castToIntegerString() on each enum? You also get comparitors and a bunch of other stuff for free.
There was a problem hiding this comment.
Unfortunately no, we are using Schema class from connect so no control on it!
| } else { | ||
| throw new KsqlException("Type is not supported: " + schema); | ||
|
|
||
| switch (schema.type()) { |
There was a problem hiding this comment.
this could also be push down do an enum on Schema.INT32.enforceInteger() etc - then you dont need this class
There was a problem hiding this comment.
Unfortunately no, we are using Schema class from connect so we cannot change it!
There was a problem hiding this comment.
@hjafarpour - I still dont see more OO modelling in the latest commit...
There was a problem hiding this comment.
Oops, I forgot to submit my comments yesterday. @bluemonk3y as I mentioned above we use Schema class from Connect package so we don't have control on the class implementation.
There was a problem hiding this comment.
We can create our own class to do this - i.e., one that wraps the Schema
There was a problem hiding this comment.
+1 - the code with have in here needs a serious refactor
This PR will unify schema checking and uses schema type in all places that we compare schemas.