Conversation
There was a problem hiding this comment.
Thanks, and sorry for the delay! I've added some improvement suggestions.
| @@ -11,9 +11,9 @@ org.eclipse.jdt.core.compiler.annotation.nullable.secondary= | |||
| org.eclipse.jdt.core.compiler.annotation.nullanalysis=disabled | |||
There was a problem hiding this comment.
This is a temporary file, so we should avoid checking it in.
There was a problem hiding this comment.
So sorry for this and I have addressed this now.
| public void removeTable(T table) { | ||
| if (this.tables.contains(table)) { | ||
| this.tables.remove(table); | ||
| for (C c : table.getColumns()) { |
There was a problem hiding this comment.
Is it actually necessary to remove the columns?
There was a problem hiding this comment.
I don't think it's necessary for CODDTest. I introduced the removeTable method because, when using a join clause in the Original query, we need the same clause in the Auxiliary query. Since the Auxiliary query is generated first, the join clauses are fixed when generating the Original query. To avoid redundant tables in the FROM clause, I remove them accordingly.
While it's not essential to remove columns in CODDTest—since they're used to generate other expressions—it might still be better to do so. If removeTable is reused by other components, it could leave behind columns in WHERE or other clauses that no longer belong to any table.
| } | ||
| } | ||
|
|
||
| public Boolean isContained(T table) { |
There was a problem hiding this comment.
I think this should return a primitive boolean value rather than an Object.
There was a problem hiding this comment.
I have addressed this.
| public int maxNumIndexes = 20; | ||
|
|
||
| @Parameter(names = { "--coddtest-model" }, description = "Apply CODDTest on expression, subquery, or random") | ||
| public String coddTestModel = "random"; |
There was a problem hiding this comment.
I think we should use an enum here.
There was a problem hiding this comment.
Yes, this will be better and I updated this.
| sb.append(")"); | ||
| } | ||
| if (!s.getGroupByClause().isEmpty()) { | ||
| if (s.getGroupByClause().size() > 0) { |
There was a problem hiding this comment.
I think we should keep the isEmpty here.
There was a problem hiding this comment.
Yes, I honestly can't remember why I made that change at the time, even though they look the same. I have addressed this now.
| return this.withClause; | ||
| } | ||
|
|
||
| public void replaceFromTable(String tableName, SQLite3Expression newFromExpression) { |
There was a problem hiding this comment.
I think a short comment to explain this method would be helpful.
There was a problem hiding this comment.
I add a short comment fot this method.
| public enum SQLite3DataType { | ||
| NULL, INT, TEXT, REAL, NONE, BINARY; | ||
|
|
||
| public static SQLite3DataType getTypeFromName(String name) { |
There was a problem hiding this comment.
I think we could use the builtin enum functionality for that (valueOf).
There was a problem hiding this comment.
I tried to use valueOf but found some name are different from type, for example integer and INT
| visit(vs.get(name).get(i)); | ||
| sb.append(" AS "); | ||
| switch(vs.get(name).get(i).getDataType()) { | ||
| case BINARY: |
There was a problem hiding this comment.
I think it would be better to implement the cast string representation directly in the data type class.
There was a problem hiding this comment.
Yes! I have addressed this.
|
|
||
| @Override | ||
| public void visit(SQLite3ResultMap tableSummary) { | ||
| // we utlize CASE WHEN THEN END here |
| } | ||
|
|
||
| @Override | ||
| public void visit(SQLite3ResultMap tableSummary) { |
There was a problem hiding this comment.
Could we perhaps extract smaller method calls for readability?
There was a problem hiding this comment.
I removed the code handling cases where an expression returns no results under the current database state, since I now discard the expression when it has no results. To ensure safety in case this method is used by other components, I’ve updated it to throw an exception if the expression yields an empty result.
|
Thank you for your feedback! I’ll make these updates as soon as possible. |
| @Parameter(names = { "--max-num-indexes" }, description = "The maximum number of indexes that can be created") | ||
| public int maxNumIndexes = 20; | ||
|
|
||
| public enum coddtest_model { |
There was a problem hiding this comment.
We should use Java naming conventions like "CODDTestModel" here and below: RANDOM, EXPRESSION, and SUBQUERY.
| sb.append(vs.get(name).get(i).getDataType().toString()); | ||
| sb.append("))"); | ||
| } | ||
| isFirstColumn = true; |
There was a problem hiding this comment.
Is this intended? I think this should be false?
There was a problem hiding this comment.
So sorry I make an error here, and fix it now
| } | ||
|
|
||
|
|
||
| public void addTable(T table) { |
There was a problem hiding this comment.
Ideally, we would also add test cases later on for these new methods.
There was a problem hiding this comment.
I add the unit tests for these new methods and an unit tests for SQLite3 CODDTest oracle.
… in SQLite3ToStringVisitor, add unit tests for AbstractTables and CODDTest for SQLite
| errors.add("misuse of aggregate"); | ||
| errors.add("misuse of window function"); | ||
| errors.add("second argument to nth_value must be a positive integer"); | ||
| errors.add("no such table"); |
There was a problem hiding this comment.
It's surprising that such an error could happen. Do you know why?
| errors.add("no such table"); | ||
| errors.add("no query solution"); | ||
| errors.add("unable to use function MATCH in the requested context"); | ||
| errors.add("[SQLITE_ERROR] SQL error or missing database (unrecognized token:"); |
There was a problem hiding this comment.
Does this indicate a syntax error?
|
The CI tests currently fail (see |
Strange, perhaps this is an issue that made it in with another PR. Could you please run |
|
For the tests to run, I think you'll need to add them here: sqlancer/.github/workflows/main.yml Lines 569 to 571 in 3863169 |
Support CODDTest for SQLite. This is a sub-task of the larger implementation tracked in #1054.