Add engine type prefix to kv_table_name

[lzxddz]

Summary by CodeRabbit

• Refactor

  • Storage subsystem now explicitly selects the EloqSql table engine for database and table-discovery operations; runtime behavior and error handling remain unchanged.
  • Internal interfaces updated to propagate the chosen engine across storage calls, improving clarity of engine routing.

• Chore

  • Submodule pointer advanced; low-risk internal wiring updates only — no user action required.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

Added an explicit table-engine parameter (txservice::TableEngine::EloqSql) to multiple storage API calls and updated the data_substrate submodule pointer from 203c8ce0c34264ec174a45188a01a3393f995aad to 25dd6a39cedb036e8ce849509245769fbcea48c7.

Changes

Cohort / File(s)	Summary
Data Substrate Submodule data_substrate	Submodule pointer updated from commit 203c8ce0c34264ec174a45188a01a3393f995aad to 25dd6a39cedb036e8ce849509245769fbcea48c7.
Storage Database Operations storage/eloq/eloq_db.cpp	Calls to UpsertDatabase, DropDatabase, FetchDatabase, and FetchAllDatabase updated to pass txservice::TableEngine::EloqSql as the first argument at each call site.
Storage Discovery Interface storage/eloq/eloq_i_s.cc	DiscoverAllTableNames call site changed to DiscoverAllTableNames(txservice::TableEngine::EloqSql, ...); public interface updated to accept an engine parameter.
Storage Handler Implementation storage/eloq/ha_eloq.cc	Call sites in drop_orphan_tmp_tables, eloq_discover_table_names, and related flows updated to pass txservice::TableEngine::EloqSql to discovery/fetch calls.

Sequence Diagram(s)

(Skipped — changes are parameter additions to existing calls and do not introduce a new multi-component sequential flow that requires visualization.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Add engine type prefix to kv_table_name #206 — Modifies the same storage call sites/signatures to add txservice::TableEngine::EloqSql as a parameter.

Suggested reviewers

• liunyl
• xiexiaoy

Poem

Hop, hop — a tiny patch I bring,
EloqSql now leads the spring.
Pointer bumped and params set,
Small hops forward — no regret. 🐇✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title 'Add engine type prefix to kv_table_name' does not match the actual changes, which involve adding engine parameters to storage operations throughout multiple files, not prefixing kv_table_name.	Update the title to accurately reflect the main changes, such as 'Add explicit engine identifier to storage operations' or similar.
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ca4b60 and 8793168.

📒 Files selected for processing (1)

• data_substrate

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

storage/eloq/ha_eloq.cc (1)

1027-1046: Strengthen name format validation; the "./" prefix check will skip engine-prefixed table names.

The full_name_sv.substr(0, 2) == "./" gate (line 1030–1031) assumes all discovered table names have this exact format. If DiscoverAllTableNames begins returning engine-prefixed names, these tables will be silently skipped. The monokey_to_marianame helper also hard-codes "./" prefix handling and cannot parse alternative formats. Consider using or enhancing a shared parser that handles multiple name formats robustly, or add explicit validation/normalization of discovered names before processing.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4d080b and 72b5c54.

📒 Files selected for processing (4)

• data_substrate
• storage/eloq/eloq_db.cpp
• storage/eloq/eloq_i_s.cc
• storage/eloq/ha_eloq.cc

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.208Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 127
File: storage/eloq/build_log_service.cmake:97-124
Timestamp: 2025-09-25T11:41:40.111Z
Learning: The `storage/eloq/build_log_service.cmake` file is specifically for the open log service build and only supports ROCKSDB as its log state backend. Unlike the enterprise version in `build_eloq_log_service.cmake`, it does not need to handle cloud-specific variants like ROCKSDB_CLOUD_S3 or ROCKSDB_CLOUD_GCS.

📚 Learning: 2025-11-07T05:12:43.208Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.208Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.

Applied to files:

• storage/eloq/eloq_db.cpp
• storage/eloq/eloq_i_s.cc
• storage/eloq/ha_eloq.cc

📚 Learning: 2025-11-07T07:10:40.633Z

Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.

Applied to files:

• storage/eloq/eloq_i_s.cc
• storage/eloq/ha_eloq.cc

🔇 Additional comments (5)

data_substrate (1)

1-1: The submodule update is properly aligned with the dependent code. All API calls in storage/eloq/eloq_db.cpp, storage/eloq/eloq_i_s.cc, and storage/eloq/ha_eloq.cc consistently use txservice::TableEngine::EloqSql as the first parameter for UpsertDatabase, DropDatabase, FetchDatabase, FetchAllDatabase, and DiscoverAllTableNames. No inconsistencies or breaking changes detected.

storage/eloq/eloq_db.cpp (1)

399-401: Good, consistent engine scoping for DB metadata ops.

All updated storage_hd calls now pass txservice::TableEngine::EloqSql uniformly, which should prevent cross-engine key collisions as intended. Consider extracting a local constexpr auto kEngine = txservice::TableEngine::EloqSql; to reduce repetition (optional).

Also applies to: 443-445, 469-470, 492-493, 519-520, 545-546, 571-572

storage/eloq/eloq_i_s.cc (1)

67-68: Verify DiscoverAllTableNames output format doesn’t break is_tmp_table() / basename().

Given the PR goal (“engine type prefix to kv_table_name”), please confirm that table_names returned here are still in the expected string format for is_tmp_table() and basename() (or update those helpers accordingly).

storage/eloq/ha_eloq.cc (2)

6650-6652: Good: FetchDatabase now scoped by engine.

Passing txservice::TableEngine::EloqSql here matches the rest of the storage API updates and should keep DB existence checks consistent with the engine’s namespace.

---

821-835: Confirm both overloads of DiscoverAllTableNames exist in tx_service after recent submodule update.

This file calls DiscoverAllTableNames with two different signatures (2-arg at line 824-825 and 4-arg at lines 1017-1019). The calls are made in semantically appropriate contexts (non-coroutine vs. coroutine contexts), but the external DataStoreHandler header cannot be inspected in this repository to verify both overloads still exist after the submodule was updated.