Add reusable connector smoke test by findepi · Pull Request #7155 · trinodb/trino

findepi · 2021-03-04T16:41:36Z

Add BaseConnectorSmokeTest, a reusable, slim, fast connector smoke test everyone always wanted, but never dared to ask for one.

findepi · 2021-03-04T16:41:48Z

Based on #6874 to avoid conflicts.

losipiuk · 2021-03-04T17:33:44Z

...trino-mysql/src/test/java/io/trino/plugin/mysql/TestGlobalTransactionMySqlConnectorTest.java

Should it be TestGlobalTransactionMySqlConnectorSmokeTest or TestGlobalTransactionMySqlSmokeTest?
There is Smoke in class name for Oracle.

TestMySqlGlobalTransactionMyConnectorSmokeTest

losipiuk

Looks reasonable

hashhar

Reviewed last two commits since the rest seems to come from another PR. Looks very reasonable.

Thanks a lot for adding this.

...e/src/test/java/io/trino/plugin/oracle/TestRemarksReportingOraclePoolConnectorSmokeTest.java

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

jirassimok · 2021-03-04T18:41:09Z

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

Even if it's not a public API, maybe a doc comment here and/or on the class would make this easier to understand.

"Return whether this behavior is enabled by default. If this behavior is based on another behavior, use the given function to determine whether that behavior is enabled."

Does the new method name (hasBehaviorByDefault) help alleviate the concern?

The names on the test class are better, but it doesn't help much on the enum. It's hard to convey the correct meaning of the fallback parameter. Even though the end effect is pretty simple, the way hasBehavior passes itself as a parameter to the enum's method makes me look twice to make sure I'm not confused every time I see it.

I'd probably call it getDefault and give it a doc comment like "get the default setting for this behavior or check the parent behavior using the given function."

Adding

/** * Determines whether a connector should be assumed to have this behavior. * * @param fallback callback to be used to inspect connector behaviors, if this behavior is assumed to be had conditionally */

here

findepi · 2021-03-04T21:06:05Z

AC

@losipiuk @hashhar @jirassimok thank you for the review!
please mark resolved when you're satisfied with the response

PTAL

hashhar

Looks good to me.

findepi · 2021-03-05T13:29:14Z

Rebased after #6874 merged, no changes otherwise.

kokosing · 2021-03-05T13:58:41Z

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

-    SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_EQUALITY,
-    SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_INEQUALITY,
+    SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_EQUALITY(SUPPORTS_PREDICATE_PUSHDOWN),
+    SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_INEQUALITY(SUPPORTS_PREDICATE_PUSHDOWN),


These three are on the same axis. Three flavors of the same feature. It looks like having support of something does not fit into boolean domain.

I don't understand the comment. What do you suggest here?

kokosing · 2021-03-05T13:58:46Z

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

+        this(true);
+    }
+
+    TestingConnectorBehavior(boolean defaultBehavior)


What do you mean by default? Connector default? Engine default? Each connector could have different default.

it's whether behavior should be assumed to be had by default

renaming to hasBehaviorByDefault, as the method name it fules

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

kokosing · 2021-03-05T14:00:46Z

...e/src/test/java/io/trino/plugin/oracle/TestOraclePoolRemarksReportingConnectorSmokeTest.java

            throws Exception
    {
-        oracleServer = new TestingOracleServer();
+        oracleServer = closeAfterClass(new TestingOracleServer());


separate commit?

that's a mistake, i will take it back

kokosing · 2021-03-05T14:03:42Z

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

+    SUPPORTS_INSERT,
+
+    SUPPORTS_DELETE(false),
+    SUPPORTS_ROW_LEVEL_DELETE(SUPPORTS_DELETE),


I don't get. Feature SUPPORTS_ROW_LEVEL_DELETE is by default set to SUPPORTS_DELETE which is by default disabled. How should I read this?

You should read this as:

if connector declares that it supports row level delete, then it should be expected to work

if connector declares it supports delete (and doesn't say anything about row level delete), then it should be expected to work

if connector declares it supports delete, but declares no support for row level delete, then it should be expected not to work

if connector declares nothing, no delete should be expected to work

if connector declares it supports delete, but declares no support for row level delete, then it should be expected not to work

You mean it supports delete and does not support row level delete. Does row level delete indicates delete?

Did you mean delete -> metadata delete and row level delete -> delete?

Does row level delete indicates delete?

yes, if a connector supports row level delete, then it does support delete in general

findepi · 2021-03-05T16:04:54Z

AC

findepi · 2021-03-05T16:52:39Z

@kokosing PTAL

jirassimok · 2021-03-05T19:41:18Z

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

+    SUPPORTS_JOIN_PUSHDOWN_WITH_DISTINCT_FROM(SUPPORTS_JOIN_PUSHDOWN),
+
+    SUPPORTS_CREATE_TABLE,
+    SUPPORTS_CREATE_TABLE_WITH_DATA(SUPPORTS_CREATE_TABLE),


Why not SUPPORTS_CREATE_TABLE_AS?

"AS" is too short, "WITH DATA" may be confusing... any other options?

SUPPORTS_CREATE_TABLE_AS_SELECT is probably better than just _AS.

Though I think WITH_DATA is probably fine. Maybe it could have a doc comment to help anyone who gets confused (some of the others would probably also benefit from doc comments, like JOIN_PUSHDOWN_WITH_DISTINCT_FROM).

(A doc comment for this one could just be {@code CREATE TABLE name AS query}.)

yes, we can add comments here!

findepi · 2021-03-05T21:17:38Z

CI #5758

findepi added the test label Mar 4, 2021

cla-bot bot added the cla-signed label Mar 4, 2021

findepi requested review from alexjo2144, electrum, hashhar, kokosing and losipiuk March 4, 2021 17:11

losipiuk reviewed Mar 4, 2021

View reviewed changes

hashhar approved these changes Mar 4, 2021

View reviewed changes

jirassimok reviewed Mar 4, 2021

View reviewed changes

findepi force-pushed the findepi/the-true-smoke-test branch from c9a136b to 6cc69b0 Compare March 4, 2021 21:03

hashhar approved these changes Mar 5, 2021

View reviewed changes

findepi force-pushed the findepi/the-true-smoke-test branch from 58b2dc6 to d42c941 Compare March 5, 2021 13:28

losipiuk approved these changes Mar 5, 2021

View reviewed changes

kokosing reviewed Mar 5, 2021

View reviewed changes

findepi mentioned this pull request Mar 5, 2021

Update JDBC driver for MemSQL #7179

Merged

jirassimok reviewed Mar 5, 2021

View reviewed changes

findepi added 2 commits March 5, 2021 22:18

Move default behavior logic to TestingConnectorBehavior

3e93636

Add connector smoke test

8249f87

findepi force-pushed the findepi/the-true-smoke-test branch from c5417d8 to 8249f87 Compare March 5, 2021 21:18

findepi merged commit 09f91d8 into trinodb:master Mar 5, 2021

findepi deleted the findepi/the-true-smoke-test branch March 5, 2021 21:18

findepi mentioned this pull request Mar 17, 2021

Use smoke test for TestGlobalTransactionMySqlConnectorTest #7019

Closed

findepi added this to the 353 milestone Mar 17, 2021

Conversation

findepi commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Mar 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

findepi commented Mar 4, 2021

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Mar 5, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Mar 5, 2021

Uh oh!

findepi commented Mar 5, 2021

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!

findepi commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

findepi commented Mar 4, 2021 •

edited

Loading