Add support for Presto#881
Conversation
|
Thanks a lot for the PR! Given that it is quite large, do you think it would be possible to first integrate the changes to the common SQLancer classes, and then the Presto implementation? |
|
I see that currently the formatting check is failing. You can use |
|
Hi, |
mrigger
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR. It's great to see how mature the Presto SQLancer implementation already is! I've added minor various suggestions. None of the suggestions is very important, so I'd be happy to merge the PR after you think you addressed those that make sense.
| // TODO: check | ||
| errors.add("io.airlift.slice.Slice cannot be cast to java.lang.Number"); | ||
| errors.add("Cannot cast java.lang.Long to io.airlift.slice.Slice"); | ||
| errors.add("Unexpected subquery expression in logical plan"); |
There was a problem hiding this comment.
This looks like an internal error?
There was a problem hiding this comment.
There are some errors that occur in certain versions of Presto.
Some of them are analyzed by the Presto team and they will not be addressed in the near future.
There was a problem hiding this comment.
In this case, I'd suggest using the pattern described at https://github.com/sqlancer/sqlancer/blob/f852f8cc08a98db34d40d20011b7c8fe47abf453/CONTRIBUTING.md#unfixed-bugs.
| private final JsonValueType jvt; | ||
| private final String val; | ||
|
|
||
| public PrestoJsonConstant() { |
There was a problem hiding this comment.
Perhaps an opportunity for a future PR could be to add a JSON implementation to Randomly?
There was a problem hiding this comment.
Yes, we could implement fully random JSON generator
| if (returnType.getPrimitiveDataType() == PrestoSchema.PrestoDataType.ARRAY) { | ||
| savedArrayType = returnType; | ||
| } | ||
| if (getNumberOfArguments() == -1) { |
There was a problem hiding this comment.
The code here looks quite similar to the one in PrestoDefaultFunction. Could we extract them to a common method? Also see my comments there (e.g., with respect to -1).
There was a problem hiding this comment.
PrestoDefaultFunction will be removed in the next PR.
This is the first Pull Request for [Presto](https://prestodb.io/) implementation. The following oracles are currently completed: * NoRec oracle * TLP Where * TLP Aggregate (basic aggregate function) ## Data types The following data types are currently supported: BOOLEAN, INT, FLOAT, DECIMAL, VARCHAR, CHAR, VARBINARY, JSON, DATE, TIME, TIMESTAMP, TIME_WITH_TIME_ZONE, TIMESTAMP_WITH_TIME_ZONE, INTERVAL_YEAR_TO_MONTH, INTERVAL_DAY_TO_SECOND, ARRAY. Others such as MAP, ROW, IPADDRESS, UID, IPPREFIX, HyperLogLog, P4HyperLogLog, KHyperLogLog, QDigest and TDigest will be supported in the future. ## Functions Large number of Presto supported are implemented, but only small number are actually used in test. Most of the functions will be supported in the future.
mrigger
left a comment
There was a problem hiding this comment.
Great, based on my perspective, the PR is ready to be merged. @branimir-vujicic, I assume this is fine, or did you still plan on any additional changes as part of this PR?
|
Thank you @mrigger, |
This is the first Pull Request for Presto implementation.
The following oracles are currently completed:
Data types
The following data types are currently supported:
BOOLEAN, INT, FLOAT, DECIMAL, VARCHAR, CHAR, VARBINARY, JSON,
DATE, TIME, TIMESTAMP, TIME_WITH_TIME_ZONE,
TIMESTAMP_WITH_TIME_ZONE, INTERVAL_YEAR_TO_MONTH,
INTERVAL_DAY_TO_SECOND, ARRAY.
Others such as MAP, ROW, IPADDRESS, UID, IPPREFIX, HyperLogLog,
P4HyperLogLog, KHyperLogLog, QDigest and TDigest
will be supported in the future.
Functions
Large number of Presto supported are implemented,
but only small number are actually used in test.
Most of the functions will be supported in the future.