Skip to content
This repository has been archived by the owner on Aug 23, 2021. It is now read-only.

Benchmark Modification For Cubrid #188

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Qianmin-Jiang
Copy link

This PR adds modifications on benchmark files to run Cubrid with TPCC, Wikipedia and TPCH.
For TPCH, it does not include any dialect and right now we directly modify the SQLs in /tpch/queries/Q{1-22}.java.

Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the requested changes.

@@ -133,7 +133,11 @@ public final PreparedStatement getPreparedStatementReturnKeys(Connection conn, S
}
// Everyone else can use the regular getGeneratedKeys() method
else if (is != null) {
pStmt = conn.prepareStatement(stmt.getSQL(), is);
if (getDatabaseType() == DatabaseType.CUBRID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to explain why you need this special clause?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -35,7 +35,7 @@
+ "from "
+ "lineitem "
+ "where "
+ "l_shipdate <= date '1998-12-01' - interval '95' day "
+ "l_shipdate <= ADDDATE('1998-12-01', interval -'95' day) "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove all of the changes that you made to the TPC-H queries for Cubrid? We will have to fix the benchmark later to add real dialect files.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we will remote these.

@@ -181,7 +181,10 @@ private void loadUsers() throws SQLException {
if(this.getDatabaseType() == DatabaseType.ORACLE) {
// Oracle handles quoted object identifiers differently, do not escape names
sql = SQLUtil.getInsertSQL(catalog_tbl, false);
} else if(this.getDatabaseType() == DatabaseType.CUBRID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments to explain what this does.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means for the generated the SQL queries, include the column names and skip the first auto_increment column.
The field is set to be auto_increment since in the insert queries in the UpdatePage, these fields are not set and they have not null constraints.
Both the loading phase and the procedures are inserting records. In the previous code, in the loading phase, it sets values for all columns. And in the procedures, it does not set value for the auto_increment fields. For Cubrid, it does not update the initial value of the auto_increment fields, while MySQL does this. So if we manually set values for these fields in the loading phase, in the execution phase Cubrid will set these values starting from some initial value which may already exist because of the loading phase. This causes the unique constraint violation. To avoid this, we skip setting these fields in the loading phase.

@@ -181,7 +181,10 @@ private void loadUsers() throws SQLException {
if(this.getDatabaseType() == DatabaseType.ORACLE) {
// Oracle handles quoted object identifiers differently, do not escape names
sql = SQLUtil.getInsertSQL(catalog_tbl, false);
} else if(this.getDatabaseType() == DatabaseType.CUBRID) {
sql = SQLUtil.getInsertSQL(catalog_tbl, true, false, 1, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments that explain these parameters.

@@ -477,15 +491,20 @@ private void loadRevision() throws SQLException {

// Insert the text
int col = 1;
textInsert.setInt(col++, rev_id); // old_id
if (this.getDatabaseType() != DatabaseType.CUBRID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old_id is an auto_increment field and we skip setting this field here.

for (Integer otherUserId : wlUser) {
ps.setInt(param, otherUserId.intValue());
ps.addBatch();
param = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change?

Copy link

@dengyuchenkit dengyuchenkit Dec 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous code, each time it creates a prepared statement, it rebinds the userId field and does not change other fields.
For Cubrid, it complains that in the generated statements, not all parameters are binded. From the log record in Cubrid, in the query that server received, only userId is binded and other params are not. So we rebind all the params here.

@@ -317,7 +317,7 @@ public void execute(Connection conn, PreparedStatement p) throws SQLException{
successful = true;
} catch (SQLException esql) {
int errorCode = esql.getErrorCode();
if (errorCode == 8177)
if (errorCode == 8177)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Cubrid specific?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this code exists in the previous code. Sorry we accidentally add a white space. We will remove that.

// sb.append(";");

return (sb.toString());
StringBuilder sb = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you change in this function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used 4 spaces as indentation here and the master branch also has 4 spaces indentation. But the version compared to has 3 indentation. So we keep the 4 spaces indentation here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants