Support CODDTest for SQLite #1181

DerZc · 2025-03-22T07:24:38Z

Support CODDTest for SQLite. This is a sub-task of the larger implementation tracked in #1054.

mrigger

Thanks, and sorry for the delay! I've added some improvement suggestions.

mrigger · 2025-04-08T15:17:31Z

.settings/org.eclipse.jdt.core.prefs

 org.eclipse.jdt.core.compiler.annotation.nonnullbydefault.secondary=
 org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jdt.annotation.Nullable
 org.eclipse.jdt.core.compiler.annotation.nullable.secondary=
 org.eclipse.jdt.core.compiler.annotation.nullanalysis=disabled


This is a temporary file, so we should avoid checking it in.

So sorry for this and I have addressed this now.

mrigger · 2025-04-08T15:18:20Z

src/sqlancer/common/schema/AbstractTables.java

+    public void removeTable(T table) {
+        if (this.tables.contains(table)) {
+            this.tables.remove(table);
+            for (C c : table.getColumns()) {


Is it actually necessary to remove the columns?

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.

mrigger · 2025-04-08T15:18:53Z

src/sqlancer/common/schema/AbstractTables.java

+        }
+    }
+
+    public Boolean isContained(T table) {


I think this should return a primitive boolean value rather than an Object.

I have addressed this.

mrigger · 2025-04-08T15:20:55Z

src/sqlancer/sqlite3/SQLite3Options.java

    public int maxNumIndexes = 20;

+    @Parameter(names = { "--coddtest-model" }, description = "Apply CODDTest on expression, subquery, or random")
+    public String coddTestModel = "random";


I think we should use an enum here.

Yes, this will be better and I updated this.

mrigger · 2025-04-08T15:21:38Z

src/sqlancer/sqlite3/SQLite3ToStringVisitor.java

            sb.append(")");
        }
-        if (!s.getGroupByClause().isEmpty()) {
+        if (s.getGroupByClause().size() > 0) {


I think we should keep the isEmpty here.

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.

mrigger · 2025-04-08T15:28:43Z

src/sqlancer/sqlite3/ast/SQLite3Select.java

+        return this.withClause;
+    }
+
+    public void replaceFromTable(String tableName, SQLite3Expression newFromExpression) {


I think a short comment to explain this method would be helpful.

I add a short comment fot this method.

mrigger · 2025-04-08T15:30:08Z

src/sqlancer/sqlite3/schema/SQLite3DataType.java

 public enum SQLite3DataType {
    NULL, INT, TEXT, REAL, NONE, BINARY;

+    public static SQLite3DataType getTypeFromName(String name) {


I think we could use the builtin enum functionality for that (valueOf).

I tried to use valueOf but found some name are different from type, for example integer and INT

mrigger · 2025-04-08T15:31:32Z

src/sqlancer/sqlite3/SQLite3ToStringVisitor.java

+                    visit(vs.get(name).get(i));
+                    sb.append(" AS ");
+                    switch(vs.get(name).get(i).getDataType()) {
+                    case BINARY:


I think it would be better to implement the cast string representation directly in the data type class.

Yes! I have addressed this.

mrigger · 2025-04-08T15:31:52Z

src/sqlancer/sqlite3/SQLite3ToStringVisitor.java

+
+    @Override
+    public void visit(SQLite3ResultMap tableSummary) {
+        // we utlize CASE WHEN THEN END here


mrigger · 2025-04-08T15:32:56Z

src/sqlancer/sqlite3/SQLite3ToStringVisitor.java

+    }
+
+    @Override
+    public void visit(SQLite3ResultMap tableSummary) {


Could we perhaps extract smaller method calls for readability?

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.

DerZc · 2025-04-09T01:09:50Z

Thank you for your feedback! I’ll make these updates as soon as possible.

mrigger · 2025-04-13T15:50:25Z

src/sqlancer/sqlite3/SQLite3Options.java

    @Parameter(names = { "--max-num-indexes" }, description = "The maximum number of indexes that can be created")
    public int maxNumIndexes = 20;

+    public enum coddtest_model {


We should use Java naming conventions like "CODDTestModel" here and below: RANDOM, EXPRESSION, and SUBQUERY.

mrigger · 2025-04-13T15:51:35Z

src/sqlancer/sqlite3/SQLite3ToStringVisitor.java

+                    sb.append(vs.get(name).get(i).getDataType().toString());
+                    sb.append("))");
+                }
+                isFirstColumn = true;


Is this intended? I think this should be false?

So sorry I make an error here, and fix it now

mrigger · 2025-04-13T15:53:02Z

src/sqlancer/common/schema/AbstractTables.java

    }

+
+    public void addTable(T table) {


Ideally, we would also add test cases later on for these new methods.

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

mrigger · 2025-04-21T14:35:42Z

src/sqlancer/sqlite3/oracle/SQLite3CODDTestOracle.java

+        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");


It's surprising that such an error could happen. Do you know why?

mrigger · 2025-04-21T14:36:01Z

src/sqlancer/sqlite3/oracle/SQLite3CODDTestOracle.java

+        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:");


Does this indicate a syntax error?

mrigger · 2025-04-21T14:42:38Z

The CI tests currently fail (see mvn -B verify -DskipTests=true). After fixing them and checking that the CI tests run CODDTest, I think we can merge the PR. I find the test oracle implementation quite complex and think there would be room for simplifying it, but it would be better to first get the PR in.

…able

mrigger · 2025-04-23T16:08:22Z

Error: Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.20.0:validate (eclipseformat) on project sqlancer: File '/home/runner/work/sqlancer/sqlancer/src/sqlancer/hive/HiveToStringVisitor.java' has not been previously formatted. Please format file (for example by invoking mvn net.revelc.code.formatter:formatter-maven-plugin:2.20.0:format) and commit before running validation! -> [Help 1]

Strange, perhaps this is an issue that made it in with another PR. Could you please run mvn formatter:format nevertheless so that the general tests pass (again)?

mrigger · 2025-04-23T16:14:05Z

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

    
                     mvn -Dtest=TestSQLitePQS test 
        
                     mvn -Dtest=TestSQLiteTLP test 
        
                     mvn -Dtest=TestSQLiteNoREC test

mrigger

Thanks, lgtm!

DerZc added 6 commits March 22, 2025 15:12

add the implementation of CODDTest for SQLite3

17952a4

fix a typo and remove some redundant code of CODDTest for sqlite3

2f0c1ee

fix the false positive in CODDTest for SQLite

b8dbd9d

fix a typo

d9579b8

add comments for SQLite3ExpressionBag

9a3a2e9

remove deleteCharAt to simplify code logic

9ead27a

mrigger reviewed Apr 8, 2025

View reviewed changes

Address the issues mentioned in the PR comments.

089d2d0

DerZc force-pushed the coddtest_sqlite branch from 986dcd1 to 089d2d0 Compare April 10, 2025 10:58

DerZc added 2 commits April 10, 2025 19:05

Address the issues mentioned in the PR comments.

8e607cc

Merge remote-tracking branch 'upstream/main' into coddtest_sqlite

5eff148

mrigger reviewed Apr 13, 2025

View reviewed changes

DerZc added 2 commits April 17, 2025 13:20

modify variabels name to follow Java naming convertions, fix an error…

d45cd02

… in SQLite3ToStringVisitor, add unit tests for AbstractTables and CODDTest for SQLite

Merge remote-tracking branch 'upstream/main' into coddtest_sqlite

0fd99d3

mrigger reviewed Apr 21, 2025

View reviewed changes

DerZc and others added 4 commits April 22, 2025 14:20

format the CODDTest for SQLite3 code

c00a19b

remove three errors, and fix a warning about the using of static vari…

dcdf7d6

…able

remove the redundant code

597b43a

Merge branch 'main' into coddtest_sqlite

294eb73

DerZc added 3 commits April 24, 2025 00:16

format hive code

b2d0284

add test of CODDTest for SQLite3 in github action

f523530

format hive for mvn verify

a9f7dce

mrigger approved these changes Apr 24, 2025

View reviewed changes

mrigger merged commit 3d9a5cc into sqlancer:main Apr 24, 2025
17 of 26 checks passed

Support CODDTest for SQLite #1181

Support CODDTest for SQLite #1181

Uh oh!

Conversation

DerZc commented Mar 22, 2025

Uh oh!

mrigger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DerZc commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrigger commented Apr 21, 2025

Uh oh!

mrigger commented Apr 23, 2025

Uh oh!

mrigger commented Apr 23, 2025

Uh oh!

mrigger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone