Support CnosDB by ZuoTiJia · Pull Request #697 · sqlancer/sqlancer · GitHub

Hello, I am from the CnosDB development team. We implemented Sqlancer of CnosDB referring to the code of Postgres in Sqlancer. This tool helped us find some bugs.

Awesome, thanks for the PR! Starting to review the PR asap. In the meanwhile, could you please execute mvn -B verify -DskipTests=true and fix the warnings?

Awesome, thanks for the PR! Starting to review the PR asap. In the meanwhile, could you please execute mvn -B verify -DskipTests=true and fix the warnings?

Thank you for reminding

Thanks a lot for the PR! Could you please take a look at my comments, which are minor issues and suggestions?

Currently, the implementation contains much effectively dead code, namely the getExpectedValue methods needed for Pivoted Query Synthesis (PQS), which is not supported as a test oracle. Would it be possible to remove these methods?

For this and other methods, is there actually a need to re-implement them? It seems that, at least for this method and the one below, the code is the same as for

sqlancer/src/sqlancer/ComparatorHelper.java

Lines 21 to 37 in 67751dc

    
           public static boolean isEqualDouble(String first, String second) { 
        
               try { 
        
                   double val = Double.parseDouble(first); 
        
                   double secVal = Double.parseDouble(second); 
        
                   return equals(val, secVal); 
        
               } catch (Exception e) { 
        
                   return false; 
        
               } 
        
           } 
        
           static boolean equals(double a, double b) { 
        
               if (a == b) { 
        
                   return true; 
        
               } 
        
               // If the difference is less than epsilon, treat as equal. 
        
               return Math.abs(a - b) < 0.001 * Math.max(Math.abs(a), Math.abs(b)) + 0.001; 
        
           }

I believe that commenting out a SQL statement should not result in any errors, so the expected errors could be empty here?

I think inheriting from

sqlancer/src/sqlancer/common/log/SQLLoggableFactory.java

Line 9 in 67751dc

public class SQLLoggableFactory extends LoggableFactory {

and overriding methods whose implementation differs would prevent some code duplication here.

I noticed that this is still unaddressed. Is it not possible to inherit because of some reason related to not using JDBC?

At present, our implementation does not use JDBC. At the beginning of development, JDBC support was not perfect, so we used the http interface, and we will optimize it later.

As an optional suggestion, I think it would be more elegant to include the currently commented-out code. If, for example, you require a random aggregate function, you could still remove the buggy aggregate functions from the selection. The general pattern is described at https://github.com/sqlancer/sqlancer/blob/67751dc3d83b880cd8c7ed53416b8b157a2ba2ea/CONTRIBUTING.md#implementing-support-for-a-new-dbms, and I see you added a CnosDBBugs class already.

Thanks. I removed the comment and applied that pattern.

Currently, the PR includes many getExpectedValue implementations, but does not support PQS, for which they are needed. Perhaps we can remove these implementations?

Thanks, I remove those dead code.

Perhaps we can remove this commented-out code?

Thanks, I remove those comment.

As an optional suggestion, as in the other instance, we could apply the CnosDBBugs pattern here.

Thanks, I remove those comment.

Thanks for your review, I removed some dead code.

Here, it would be ideal to directly create an ExpectedError object rather than a list.

Almost ready to merge! Could you please still have a look at the other comments?

The most recent commit added a couple of Main.nrUnsuccessfulActions.addAndGet(1); and similar calls like this. I believe we should remove them to not let the two comparator helper classes diverge until we fix the code duplication problem.

I think that this method should be called getExpectedExpressionErrors since all the errors seem specific to expressions? This is something that could be done in a future PR.

LGTM, thanks a lot again for your contribution! It would be great to add CI tests (e.g., see

sqlancer/.github/workflows/main.yml

Line 105 in 39c48cd

name: DBMS Tests (CockroachDB)

) as one of the next steps, so we make sure that the implementation does not break in the future.

The branch is currently out-of-date. Could please merge latest the latest master? It seems I don't have permissions to do so.

Add Cnosdb

* add query execute count * refactor expectedError

LGTM, thanks a lot again for your contribution! It would be great to add CI tests (e.g., see

sqlancer/.github/workflows/main.yml

Line 105 in 39c48cd

name: DBMS Tests (CockroachDB)

) as one of the next steps, so we make sure that the implementation does not break in the future.

we'll add it soon

Thanks a lot!

ZuoTiJia force-pushed the master branch from 0663473 to ae016f8 Compare January 19, 2023 07:03

mrigger reviewed Jan 19, 2023

View reviewed changes

ZuoTiJia requested a review from mrigger January 20, 2023 03:50

mrigger reviewed Jan 21, 2023

View reviewed changes

mrigger reviewed Jan 28, 2023

View reviewed changes

ZuoTiJia requested a review from mrigger January 28, 2023 09:36

mrigger approved these changes Jan 28, 2023

View reviewed changes

ZuoTiJia and others added 7 commits January 28, 2023 19:09

Add Cnosdb (#1 )

7fb66ef

Add Cnosdb

cnosdb: fix check style error

b69492a

cnosdb: fix compiler error

63e5f31

cnosdb: remove unused

624ba5a

cnosdb: remove some pqs impl

a449be0

cnosdb: remove dead code

8493a4e

* add query execute count * refactor expectedError

cnosdb: remove unsuccessful count

8ec69bf

ZuoTiJia force-pushed the master branch from 7c1e6e3 to 8ec69bf Compare January 28, 2023 11:09

mrigger merged commit 354d591 into sqlancer:master Jan 28, 2023

ZuoTiJia mentioned this pull request Jan 29, 2023

SQLancer Support cnosdb/cnosdb#860

Closed

4 tasks

+                      String queryString = query.getLogString();
+                      String newQueryString = "-- " + queryString;
+                      ExpectedErrors errors = new ExpectedErrors();
+                      errors.addAll(CnosDBExpectedError.expectedErrors());

+                          state.getState().getLocalState().log(String.format("%s\n%s", firstQueryString, secondQueryString));
+                          String assertionMessage = String.format("the size of the result sets mismatch (%d and %d)!\n%s\n%s",
+                                  resultSet.size(), secondResultSet.size(), firstQueryString, secondQueryString);
+                          Main.nrUnsuccessfulActions.addAndGet(1);

	public static boolean isEqualDouble(String first, String second) {
	try {
	double val = Double.parseDouble(first);
	double secVal = Double.parseDouble(second);
	return equals(val, secVal);
	} catch (Exception e) {
	return false;
	}
	}

	static boolean equals(double a, double b) {
	if (a == b) {
	return true;
	}
	// If the difference is less than epsilon, treat as equal.
	return Math.abs(a - b) < 0.001 * Math.max(Math.abs(a), Math.abs(b)) + 0.001;
	}

+              import sqlancer.common.query.ExpectedErrors;
+              import sqlancer.common.query.Query;
+              public class CnosDBLoggableFactory extends LoggableFactory {

+                      },
+                      SUM(CnosDBDataType.INT, CnosDBDataType.DOUBLE, CnosDBDataType.UINT), APPROX_MEDIAN(CnosDBDataType.DOUBLE);
+                      // Currently these aggregate functions have bugs https://github.com/cnosdb/cnosdb/issues/786

+                  }
+                  @Override
+                  public CnosDBConstant getExpectedValue() {

+                          }
+                      };
+                      // NULL_IF(2, "nullif") {

+                  DATA_PART("date_part", CnosDBDataType.INT, CnosDBDataType.STRING, CnosDBDataType.TIMESTAMP);
+                  // because bug of arrow-csv https://github.com/apache/arrow-rs/issues/3547
+                  // NOW("now", CnosDBDataType.TIMESTAMP),

+                  }
+                  public static List<String> expectedErrors() {
+                      List<String> errors = new ArrayList<>();

+                      ERRORS.add("Coercion from");
+                  }
+                  public static ExpectedErrors expectedErrors() {

Support CnosDB by ZuoTiJia · Pull Request #697 · sqlancer/sqlancer · GitHub

Uh oh!

Support CnosDB - #697

Support CnosDB#697mrigger merged 7 commits intosqlancer:mastersqlancer/sqlancer:masterfrom cnosdb:mastercnosdb/sqlancer:masterCopy head branch name to clipboard

Conversation

ZuoTiJia commented Jan 18, 2023

Uh oh!

mrigger commented Jan 18, 2023

Uh oh!

ZuoTiJia commented Jan 19, 2023

Uh oh!

mrigger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

ZuoTiJia commented Jan 20, 2023

Uh oh!

Choose a reason for hiding this comment

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!

mrigger left a comment

Choose a reason for hiding this comment

Uh oh!

mrigger commented Jan 28, 2023

Uh oh!

ZuoTiJia commented Jan 28, 2023

Uh oh!

mrigger commented Jan 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Support CnosDB#697
mrigger merged 7 commits into
sqlancer:masterfrom
cnosdb:master