Add cross-partition scan options #1294

jnmt · 2023-11-16T04:05:28Z

Description

This PR introduces cross-partition scan options and the related errors and warnings so that developers can know ScanAll's behavior is different from the regular Scan and make sure to use it explicitly (at their own risk for NoSQL databases).

Related issues and/or PRs

Add relational scan for JDBC storage and consensus commit transaction #900

Changes made

Add three options in DatabaseConfig
- cross-partition scan: ScanAll without filtering and ordering
- cross-partition scan with filtering: ScanAll with conjunctions (where clause)
- cross-partition scan with ordering: ScanAll with ordering
- To enable the last two, you need to enable the first one
- Why these three? This granularity will help users understand the support features in their databases, especially when we implement filtering for the NoSQL cross-partition scan.
Add warnings when you enable cross-partition scan with serializable isolation
Change the following for each storage
- Add config check for NoSQL databases if cross-partition scan filtering and ordering are both disabled
- Add scan operation check if cross-partition scan options are appropriately enabled (in OperationChecker)
- Make the storage name public in the config class to refer to it in a single place because some storage names are required for checking the config and warnings

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes. (Will be done in another PR)
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

We still have a lot of the word RelationalScan in tests, but keep them as is for now
See inline comments for other notes

Release notes

Added cross-partition scan options

jnmt · 2023-11-16T04:35:53Z

core/src/test/java/com/scalar/db/storage/cassandra/CassandraTest.java

We no longer need this test class because it only tests if the scan is relational or not, and now we test it in OperationChecker instead. Ditto for other storage.

jnmt · 2023-11-16T04:46:58Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcService.java

-      }
-    } else {
-      return buildSelectQueryForScan(scan, tableMetadata);
+      return buildSelectQuery((ScanAll) scan, tableMetadata);


After introducing the cross-partition scan concept, I was convinced that we don't have to distinguish ScanAll and RelationalScan anymore. This is a kind of refactoring, but I included this since it is related to re-organizing relational scan terminology.

jnmt · 2023-11-16T06:03:53Z

core/src/main/java/com/scalar/db/config/DatabaseConfig.java

@@ -90,6 +98,18 @@ protected void load() {
    activeTransactionManagementExpirationTimeMillis =
        getLong(getProperties(), ACTIVE_TRANSACTION_MANAGEMENT_EXPIRATION_TIME_MILLIS, -1);
    defaultNamespaceName = getString(getProperties(), DEFAULT_NAMESPACE_NAME, null);
+    crossPartitionScanEnabled = getBoolean(getProperties(), CROSS_PARTITION_SCAN, false);


The default value is false in v4.0.0 or later, but it should be true in v3.x for backward compatibility and warned when using it with SERIALIZABLE. I will handle this after this PR is merged to the master.

komamitsu · 2023-11-17T10:10:28Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitConfig.java

+
+  private void warnCrossPartitionScan(String storageName) {
+    logger.warn(
+        "Enabling cross-partition scan for {} in production is not recommended "


[trivial] I imagined what the message would be like (e.g., "Enabling cross-partition scan for cosmos in production ..."), and I think adding quotations might be a bit better.

Suggested change

"Enabling cross-partition scan for {} in production is not recommended "

"Enabling cross-partition scan for '{}' in production is not recommended "

Fixed in 7bffcbb. Thank you!

komamitsu · 2023-11-17T10:40:21Z

core/src/main/java/com/scalar/db/storage/cassandra/Cassandra.java

+    if (config.isCrossPartitionScanFilteringEnabled()
+        || config.isCrossPartitionScanOrderingEnabled()) {
+      throw new IllegalArgumentException(
+          "Cross-partition scan with filtering or ordering is not supported in Cassandra");


I think this is related to https://github.com/scalar-labs/scalardb/pull/1294/files#diff-660ca76c9b64d94000e0854e2a18fdc5e6a69c028a74b8cacfa4ef733d4604c2L111, but sorry, I forgot why these types of scan were prohibitted. Could you tell me that?

Thanks for the question. Basically, it's hard to push down all kinds of filtering and ordering to NoSQL databases. But for filtering, we can support ScalarDB-side filtering in the near future since we already have a logic for evaluating all types of our predicates.

[skip ci]

brfrn169

Left a minor comment, but LGTM! Thank you!

brfrn169 · 2023-11-20T02:11:10Z

core/src/main/java/com/scalar/db/storage/jdbc/JdbcConfig.java

@@ -81,12 +83,14 @@ public class JdbcConfig {
  public JdbcConfig(DatabaseConfig databaseConfig) {
    String storage = databaseConfig.getStorage();
    String transactionManager = databaseConfig.getTransactionManager();
-    if (!"jdbc".equals(storage) && !"jdbc".equals(transactionManager)) {
+    if (!storage.equals(STORAGE_NAME) && !transactionManager.equals(STORAGE_NAME)) {


Currently, when we set the transactionManager config to jdbc, the storage config is not required. So the storage config can be null. So we need to use the STORAGE_NAME.equals(...) style to avoid NullPointerExceptiom:

Suggested change

if (!storage.equals(STORAGE_NAME) && !transactionManager.equals(STORAGE_NAME)) {

if (!STORAGE_NAME.equals(storage) && !STORAGE_NAME.equals(transactionManager)) {

Or we also use Objects.equals() as well. Either is fine for me. Thanks.

@brfrn169 Thank you for pointing it out. I think cassandra is always set to the storage by default when we create DatabaseConfig, it will never be null. If I understand it correctly, is the assertion better?

Ah yes, you are right. I don't remember why we used that reverse style ("jdbc".equals(storage)) originally. It looks like we have default values for storage and transactionManager, so we don't need to care about NullPointerException. So let's keep it as it is. Thanks.

komamitsu

LGTM! 👍

feeblefakie

LGTM! Thank you!

kota2and3kan

LGTM! Thank you!
(Sorry for my late review... 🙇‍♀️)

jnmt · 2023-11-24T03:17:53Z

@Torch3333 I will merge this, but PTAL when you are back.

Add cross-partition scan options

118aef8

jnmt commented Nov 16, 2023

View reviewed changes

Fix integration tests for server

66cd3a2

jnmt self-assigned this Nov 16, 2023

jnmt added the improvement label Nov 16, 2023

jnmt requested review from brfrn169, feeblefakie, Torch3333, kota2and3kan and komamitsu November 16, 2023 05:57

jnmt commented Nov 16, 2023

View reviewed changes

komamitsu reviewed Nov 17, 2023

View reviewed changes

Fix message

7bffcbb

[skip ci]

jnmt requested a review from komamitsu November 20, 2023 02:00

brfrn169 approved these changes Nov 20, 2023

View reviewed changes

komamitsu approved these changes Nov 20, 2023

View reviewed changes

jnmt mentioned this pull request Nov 21, 2023

Update documents for cross-partition scan and import feature #1301

Merged

6 tasks

feeblefakie approved these changes Nov 21, 2023

View reviewed changes

kota2and3kan approved these changes Nov 22, 2023

View reviewed changes

jnmt and others added 3 commits November 24, 2023 10:21

Merge branch 'master' into add-cross-partition-scan-option

36e1010

Fix spotbugs issues

0ab1406

Merge branch 'master' into add-cross-partition-scan-option

f670f4f

jnmt merged commit 12eafa2 into master Nov 24, 2023
23 checks passed

jnmt deleted the add-cross-partition-scan-option branch November 24, 2023 03:18

feeblefakie mentioned this pull request Nov 24, 2023

Backport to branch(3) failed: Add cross-partition scan options #1322

Closed

jnmt mentioned this pull request Nov 24, 2023

Backport to branch(3) : Add cross-partition scan options #1324

Merged

jnmt mentioned this pull request May 21, 2024

Fix default value of cross-partition scan in docs #1753

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cross-partition scan options #1294

Add cross-partition scan options #1294

jnmt commented Nov 16, 2023 •

edited

Loading

jnmt Nov 16, 2023 •

edited

Loading

jnmt Nov 16, 2023

jnmt Nov 16, 2023

komamitsu Nov 17, 2023

jnmt Nov 20, 2023

komamitsu Nov 17, 2023

jnmt Nov 20, 2023

brfrn169 left a comment

brfrn169 Nov 20, 2023

jnmt Nov 20, 2023

brfrn169 Nov 20, 2023

komamitsu left a comment

feeblefakie left a comment

kota2and3kan left a comment

jnmt commented Nov 24, 2023

	"Enabling cross-partition scan for {} in production is not recommended "
	"Enabling cross-partition scan for '{}' in production is not recommended "

	if (!storage.equals(STORAGE_NAME) && !transactionManager.equals(STORAGE_NAME)) {
	if (!STORAGE_NAME.equals(storage) && !STORAGE_NAME.equals(transactionManager)) {

Add cross-partition scan options #1294

Add cross-partition scan options #1294

Conversation

jnmt commented Nov 16, 2023 • edited Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

jnmt Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

kota2and3kan left a comment

Choose a reason for hiding this comment

jnmt commented Nov 24, 2023

jnmt commented Nov 16, 2023 •

edited

Loading

jnmt Nov 16, 2023 •

edited

Loading