Conversation
Signed-off-by: Abhinav Gyawali <[email protected]>
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
We can add methods for generating random values to the compiler if you can reuse the data types there. We have literals for all these types - what you call constants.
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import com.yugabyte.jdbc.EscapeSyntaxCallMode; |
There was a problem hiding this comment.
That is not needed, I didn't even notice that.
Will remove.
| case DOUBLE: | ||
| case TINYINT: | ||
| case SMALLINT: | ||
| case FLOAT: |
There was a problem hiding this comment.
I changed this type to only represent the primitive types, so FLOAT represents DOUBLE, BOOL, and INT represents all the different sizes of the integer types.
This is similar to the implementation done by other databases in sqlancer.
| } | ||
| } | ||
|
|
||
| public static class FelderaCompositeDataType { |
There was a problem hiding this comment.
this is not general enough, MAP has two generic arguments.
There was a problem hiding this comment.
Maybe call this GenericType?
Too bad we aren't reusing the structures we already have in the compiler.
| return FelderaConstant.getRandomConstant(globalState, this); | ||
| } | ||
|
|
||
| public boolean isNumeric() { |
There was a problem hiding this comment.
This looks wrong.
An array is not numeric.
There was a problem hiding this comment.
This method is not specific to arrays.
It returns true for int types, FP types and decimal.
| return this.elementType; | ||
| } | ||
|
|
||
| public static FelderaCompositeDataType getBooleanType() { |
There was a problem hiding this comment.
I don't understand what this class is supposed to be.
Maybe it's a type factory?
There was a problem hiding this comment.
this code should be in a separate factory class, which may also implement a singleton pattern.
There was a problem hiding this comment.
This is designed represents all datatypes in feldera (doesn't quite support map yet).
Similar to other implementations:
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
this suggests that Type should be an interface and these all subclasses.
What are the constraints imposed from SqlLancer?
Maybe you really should reuse the compiler type hierarchy.
|
|
||
| public enum FelderaAggregateFunction { | ||
| AVG(FelderaSchema.FelderaDataType.INT, FelderaSchema.FelderaDataType.DOUBLE), | ||
| AVG(FelderaSchema.FelderaDataType.INT, FelderaSchema.FelderaDataType.FLOAT), |
There was a problem hiding this comment.
we don't even support FLOAT in SQL, this is very confusing.
There was a problem hiding this comment.
With FLOAT, we represent both DOUBLE and REAL here.
There was a problem hiding this comment.
Many functions work on double but not on real.
| return new FelderaNullConstant(); | ||
| case TIME: | ||
| return FelderaTimeConstant.getRandom(globalState); | ||
| return new FelderaCast(FelderaTimeConstant.getRandom(globalState), type); |
There was a problem hiding this comment.
is this because now time literals are strings?
|
How about I try to rework this code to reuse the compiler ir? We should pack the ir in a separate jar and publish it too. |
I think that would be nice. I wanted to do that, but couldn't figure out how to do it nicely. |
* disables NATURAL join for now as it produces a lot of compilation errors Signed-off-by: Abhinav Gyawali <[email protected]>
2 participants
adds support for testing array types and more functions that are available in feldera