Seafowl CLI initial implementation #468

gruuya · 2023-11-21T14:39:14Z

Add a basic CLI implementation that supports searchable history, multi-line comments and multi-statement queries:

gruuya@markogrujics-MacBook-Pro seafowl % cargo run -- --cli                                            
   Compiling seafowl v0.5.2 (/Users/gruuya/Splitgraph/seafowl)
    Finished dev [unoptimized + debuginfo] target(s) in 5.06s
     Running `target/debug/seafowl --cli`
default> create table t as values (1, 'one'), (2, 'two');
Error during planning: Table "t" already exists
default> drop table t;
Time: 0.006s
default> create table t as values (1, 'one'), (2, 'two');
Time: 0.023s
default> select *
from t
where column1 > 1;
+---------+---------+
| column1 | column2 |
+---------+---------+
| 2       | two     |
+---------+---------+
Time: 0.024s
default> select column1 from t; select column2 from t;
+---------+
| column1 |
+---------+
| 1       |
| 2       |
+---------+
+---------+
| column2 |
+---------+
| one     |
| two     |
+---------+
Time: 0.018s

Validation and highlighting can be covered separately.

Closes #467

mildbyte · 2023-11-28T00:44:36Z

README.md

+## CLI
+
+Seafowl also provides a CLI to accommodate frictionless prototyping, troubleshooting and testing of
+the core features :


nit

Suggested change

the core features :

the core features:

mildbyte · 2023-11-28T00:45:13Z

README.md

+
+It does so by circumventing Seafowl's primary HTTP interface, which involves properly formatting
+HTTP requests with queries, authentication, as well as dealing with potentially faulty networking
+setup, which can sometimes be too tedious for a quick manual interactive session.


Suggested change

setup, which can sometimes be too tedious for a quick manual interactive session.

setups, which can sometimes be too tedious for a quick manual interactive session.

mildbyte · 2023-11-28T00:50:06Z

src/cli/helper.rs

+    #[allow(clippy::if_same_then_else)]
+    fn validate_input(&self, input: &str) -> Result<ValidationResult> {
+        if input.ends_with(';') {
+            // TODO: actually perform validation here
+            Ok(ValidationResult::Valid(None))
+        } else if input.starts_with('\\') {
+            // command
+            Ok(ValidationResult::Valid(None))


minor: could fold into one clause and avoid the clippy pragma

Suggested change

#[allow(clippy::if_same_then_else)]

fn validate_input(&self, input: &str) -> Result<ValidationResult> {

if input.ends_with(';') {

// TODO: actually perform validation here

Ok(ValidationResult::Valid(None))

} else if input.starts_with('\\') {

// command

Ok(ValidationResult::Valid(None))

fn validate_input(&self, input: &str) -> Result<ValidationResult> {

if input.ends_with(';') || input.starts_with('\\') { // TODO: actually perform validation here for ;

Ok(ValidationResult::Valid(None))

I left it broken up intentionally as we'd need validation only in the first step, though come to think of it that's probably not necessary as validation is also performed once input is entered (i.e. during parsing/planning).

mildbyte · 2023-11-28T00:53:50Z

src/cli/mod.rs

+    pub async fn repl_loop(&self) -> rustyline::Result<()> {
+        let mut rl = Editor::new()?;
+        rl.set_helper(Some(CliHelper {}));
+        rl.load_history(".history").ok();


minor: should be a constant

mildbyte · 2023-11-28T00:54:35Z

src/cli/mod.rs

+            }
+        }
+
+        rl.save_history(".history")


same here with the constant

mildbyte · 2023-11-28T00:58:05Z

src/cli/mod.rs

+    }
+
+    // Interactive loop for running commands from a CLI
+    pub async fn repl_loop(&self) -> rustyline::Result<()> {


nit: repl is already a read-evaluate-print-loop, so repl_loop is redundant 😅

good point, renaming 😄

mildbyte · 2023-11-28T01:00:19Z

tests/cli/mod.rs

+
+const TEST_CONFIG_FILE: &str = "seafowl-test.toml";
+
+fn setup_temp_config_and_data_dir() -> std::io::Result<TempDir> {


Hm, we don't have any of this as an existing fixture in integration tests? I guess we always inject a pre-created context instead of setting up a config file for Seafowl.

Is there a specific reason we have to use on-disk state here instead of in-memory? Doesn't seem like the CLI tests care about persistence when restarting the CLI anyway.

Hm, we don't have any of this as an existing fixture in integration tests? I guess we always inject a pre-created context instead of setting up a config file for Seafowl.

That is correct, we didn't have this before. Typically we use

seafowl/tests/statements/mod.rs

Lines 50 to 53 in 3d428c9

/// Make a SeafowlContext that's connected to a real PostgreSQL database

async fn make_context_with_pg(

object_store_type: ObjectStoreType,

) -> (DefaultSeafowlContext, Option<TempDir>) {

to get a Seafowl context object directly.

Is there a specific reason we have to use on-disk state here instead of in-memory? Doesn't seem like the CLI tests care about persistence when restarting the CLI anyway.

In principle there's no difference for this particular test whether we use local FS or in-mem storage. However, given that the test starts up the Seafowl bin as an external command there's no way to supply the config programmatically to it directly, short of saving a config file on disk and then supplying the config file via the -c arg

let temp_dir = setup_temp_config_and_data_dir()?; let mut cmd = Command::cargo_bin("seafowl").expect("Seafowl bin exists"); cmd.arg("--cli") .arg("-c") .arg(temp_dir.path().join(TEST_CONFIG_FILE)) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped());

Since we need the temp dir for storing the config anyway I just went with the local FS storage in there as well.

I guess an alternative is to extend make_context_with_pg such that it always generates a temp dir (not only in the case of ObjectStoreType::Local as now), and then also persist the generated config file there.

I guess an alternative is to extend make_context_with_pg such that it always generates a temp dir (not only in the case of ObjectStoreType::Local as now), and then also persist the generated config file there.

Ok, I'll go ahead with the merge, and then potentially address this in the follow-up PRs.

Seafowl CLI initial implementation

1038fef

gruuya self-assigned this Nov 21, 2023

gruuya requested a review from mildbyte November 21, 2023 14:39

Apply some linting to the changes

b3f5258

gruuya force-pushed the seafowl-cli-impl branch from 73e5790 to b3f5258 Compare November 21, 2023 14:53

Add basic CLI tests

5dad4d4

gruuya force-pushed the seafowl-cli-impl branch 2 times, most recently from 596e312 to 7216c96 Compare November 22, 2023 14:15

Extend README with info on the new CLI option

e76e4c1

gruuya force-pushed the seafowl-cli-impl branch from 7216c96 to e76e4c1 Compare November 22, 2023 14:33

Implement the help cli command

dcb92e1

mildbyte approved these changes Nov 28, 2023

View reviewed changes

Rename main CLI func and extract history file name to a const

6ac58da

gruuya merged commit 5a76ce9 into main Nov 28, 2023

gruuya deleted the seafowl-cli-impl branch November 28, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Seafowl CLI initial implementation #468

Seafowl CLI initial implementation #468

Uh oh!

gruuya commented Nov 21, 2023 •

edited

Loading

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

gruuya Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

gruuya Nov 28, 2023

Uh oh!

mildbyte Nov 28, 2023

Uh oh!

gruuya Nov 28, 2023 •

edited

Loading

Uh oh!

gruuya Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	setup, which can sometimes be too tedious for a quick manual interactive session.
	setups, which can sometimes be too tedious for a quick manual interactive session.


		const TEST_CONFIG_FILE: &str = "seafowl-test.toml";

		fn setup_temp_config_and_data_dir() -> std::io::Result<TempDir> {

	/// Make a SeafowlContext that's connected to a real PostgreSQL database
	async fn make_context_with_pg(
	object_store_type: ObjectStoreType,
	) -> (DefaultSeafowlContext, Option<TempDir>) {

Seafowl CLI initial implementation #468

Seafowl CLI initial implementation #468

Uh oh!

Conversation

gruuya commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

gruuya Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gruuya commented Nov 21, 2023 •

edited

Loading

gruuya Nov 28, 2023 •

edited

Loading