-
Notifications
You must be signed in to change notification settings - Fork 3
Add ClientService and CLI for Table Store #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new ClientService and CLI for Table Store, providing command-line tools for interacting with the table store functionality. The implementation includes both service layer abstractions and command-line interfaces for executing statements, registering contracts, and validating the ledger.
- Adds ClientService and ClientServiceFactory classes to provide a higher-level API for table store operations
- Implements CLI commands for statement execution, contract registration, and ledger validation
- Extends existing client utilities to support the new table store functionality
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
table-store/src/main/java/com/scalar/dl/tablestore/client/service/ClientService.java | Core service class providing table store operations like statement execution and validation |
table-store/src/main/java/com/scalar/dl/tablestore/client/service/ClientServiceFactory.java | Factory class for creating ClientService instances with proper configuration |
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/TableStoreCommandLine.java | Main CLI entry point with grouped subcommands for table store operations |
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/StatementExecution.java | CLI command for executing PartiQL statements |
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/LedgerValidation.java | CLI command for validating table schemas and records in the ledger |
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/ContractsRegistration.java | CLI command for registering all necessary table store contracts |
table-store/src/main/java/com/scalar/dl/tablestore/client/model/StatementExecutionResult.java | Model class wrapping contract execution results for statement operations |
table-store/src/main/java/com/scalar/dl/tablestore/client/error/TableStoreClientError.java | Adds error code for multiple statements not supported |
table-store/build.gradle | Adds CLI application configuration and picocli dependency |
client/src/main/java/com/scalar/dl/client/util/Common.java | Adds utility method for loading class bytes from resources |
client/src/main/java/com/scalar/dl/client/tool/CommonOptions.java | Makes fields protected for inheritance by table store CLI classes |
client/src/main/java/com/scalar/dl/client/tool/Common.java | Makes validation and error printing methods public for reuse |
table-store/src/main/java/com/scalar/dl/tablestore/client/model/StatementExecutionResult.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @jnmt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new ClientService
and a command-line interface (CLI) specifically designed for interacting with a "Table Store" abstraction built on top of the existing Ledger system. The primary goal is to provide a more intuitive, SQL-like interface (using PartiQL) for data manipulation and validation within the Ledger, abstracting away the complexities of direct contract interactions.
Highlights
- New Table Store Client Service: A comprehensive
ClientService
is added to thetable-store
module, offering a high-level API for managing and querying data in the Table Store. This service streamlines operations by handling the underlying contract execution. - PartiQL Statement Processing: The new
ClientService
enables execution of SQL-like PartiQL statements (e.g.,INSERT
,SELECT
,UPDATE
) which are internally translated and executed as generic contracts on the Ledger. - Dedicated Command-Line Interface (CLI): A new
scalardl-table-store
executable CLI tool is introduced. It provides subcommands for registering necessary Table Store contracts, executing PartiQL statements, and validating the integrity of table schemas and records in the Ledger. - Enhanced Client Utilities: Existing client-side utilities are augmented with new functionalities, including a mechanism to load class bytecode for dynamic contract registration and a new error type to handle class loading failures.
- Expanded Contract Support: New contract constants (
ShowTables
,GetHistory
) are added, indicating extended capabilities for listing available tables and retrieving historical data through the Table Store interface.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant new feature: a ClientService
and a command-line interface (CLI) for a Table Store. The changes are extensive, including new services, data models, and CLI commands, along with a comprehensive set of tests.
The overall structure is well-designed, with a clear separation of concerns between the service layer and the CLI tools. The code is generally of high quality. However, I've identified a few issues that should be addressed:
- A critical bug in an
equals
method that could lead to aNullPointerException
. - An inconsistency in exception handling that could result in unhandled runtime exceptions.
- A performance issue related to the repeated creation of expensive objects in one of the new CLI commands.
Addressing these points will improve the correctness and robustness of the new functionality.
table-store/src/main/java/com/scalar/dl/tablestore/client/model/StatementExecutionResult.java
Outdated
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/StatementExecution.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments and suggestions. PTAL!
client/src/main/java/com/scalar/dl/client/error/ClientError.java
Outdated
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/LedgerValidation.java
Outdated
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/LedgerValidation.java
Outdated
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/LedgerValidation.java
Outdated
Show resolved
Hide resolved
table-store/src/main/java/com/scalar/dl/tablestore/client/tool/StatementExecution.java
Outdated
Show resolved
Hide resolved
SecretRegistration.class, | ||
StatementExecution.class, | ||
}, | ||
description = {"These are ScalarDL Table Store commands used in various situations:"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, are we officially going to capitalize Table Store
as a proper noun? If not, I think we should lowercase it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I just used it, similar to ScalarDB Cluster
, but let me discuss it with Hiro.
Co-authored-by: Josh Wong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!🙇🏻♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
This PR adds client service APIs and CLIs to interact with ScalarDL as a table store.
Related issues and/or PRs
Changes made
ClientServiceFactory
that createsClientService
for the table store and automatically registers the client identity (certificate or secret) and necessary contracts.ClientService
has a new interface to execute a PartiQL statement instead of executing a contract.ClientService
also has dedicated validation interfaces for table schema, records, and index records.Checklist
Additional notes (optional)
scalardl-table-store
command is separated from thescalardl
command inclient
due to a kind of circular dependency issue. However, it can probably be integrated by introducing thecli
submodule in future refactoring. Integrating all the features into a single CLI might cause confusion (e.g., which register-cert commands should I use?), though.Release notes
Added ClientService and CLI for Table Store.