Support CODDTest in sqlancer by DerZc · Pull Request #1054 · sqlancer/sqlancer
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Added some initial comments. I think it's quite challenging to review such a large PR. Do you think we could split it into multiple smaller ones (referencing this as the main PR)? For example, as a first step, we could add some of the new expression nodes and the visit methods.
| public int maxNumIndexes = 20; | ||
|
|
||
| @Parameter(names = { "--coddtest-model" }, description = "Apply CODDTest on expression, subquery, or random") | ||
| public String coddTestModel = "random"; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could use a similar strategy as the TLP oracles where we have separate enums?
| } | ||
| sb.append(", "); | ||
| } | ||
| sb.deleteCharAt(sb.length() - 1); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting previous parts seems quite complicated. Can we simplify this (and similar logic in other files)?
| } | ||
|
|
||
| @Override | ||
| public String toStringForComparison() { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems quite complicated. Can we simplify and/or document this?
| @@ -0,0 +1,24 @@ | |||
| package sqlancer.cockroachdb.ast; | |||
|
|
|||
| public class CockroachDBWithClasure implements CockroachDBExpression { | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the name mean?
| errors.add("must appear in the GROUP BY clause or be used in an aggregate function"); | ||
| errors.add("must appear in the GROUP BY clause or must be part of an aggregate function"); | ||
| errors.add("GROUP BY term out of range - should be between"); | ||
| errors.add("INTERNAL Error: Failed to bind column reference"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal errors are bugs, so we should not ignore them.
| import sqlancer.duckdb.DuckDBSchema.DuckDBCompositeDataType; | ||
| import sqlancer.duckdb.ast.DuckDBConstant.DuckDBNullConstant; | ||
|
|
||
| public class DuckDBConstantWithType implements DuckDBExpression { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we achieve the same with a cast expression to which we pass a constant?
| } | ||
| } | ||
|
|
||
| public static class SQLite3ExpressionBag extends SQLite3Expression { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some of the newly added classes, it's not immediately clear what this looks like at a SQL level. would it be possible to add a comment for such cases?