Skip to content
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

feat: update to sql engines code #3576

Merged
merged 26 commits into from
Jan 30, 2025

Conversation

Light2Dark
Copy link
Collaborator

@Light2Dark Light2Dark commented Jan 26, 2025

📝 Summary

I branched out to experiment. mo.sql can execute most SQL statements. Added some frontend.

Todo:

  • no longer existing connections aren't being deleted
  • update selection input display, icons etc (see @/mscolnick's comment below) [might not do this here]
  • fix/add some tests
  • add some docs on how to use this
  • sql regex doesn't handle different ordering of engine & output params [probs won't do it now too]

future work:

  • handle data source panel, sql parsing, so that dataset info is obtained from custom sql engines

🔍 Description of Changes

  • by default, frontend will set duckdb as engine (this is hardcoded in frontend, not provided by backend)
  • added a new atom to store the latestEngineSelected, so it's sort of "smart" when you create new cells

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

Copy link

vercel bot commented Jan 26, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 0:48am

Copy link

vercel bot commented Jan 26, 2025

@Light2Dark is attempting to deploy a commit to the marimo Team on Vercel.

A member of the Team first needs to authorize it.

@Light2Dark Light2Dark changed the title update to sql engines code feat: update to sql engines code Jan 26, 2025
</Tooltip>
</TooltipProvider>
</>
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to you on design, i was thinking we could do a non-native <Select>. we could make this fancier with logos (default to lucide database). and then put the help in the bottom (styled differently than the rest of the items).

we can also do this in a followup

Copy link
Collaborator Author

@Light2Dark Light2Dark Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, yeah thinking of doing it in a follow up. Need to hook it up to the code execution too.

could I get your thoughts on dataSourceConnections being lost on refresh? Should this be saved in local storage / somewhere

Copy link
Contributor

@mscolnick mscolnick Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that info should be stored in the session_View.py. it looks like i started to add it, but didnt quite finish:

    @property
    def operations(self) -> list[MessageOperation]:
        all_ops: list[MessageOperation] = []
        if self.cell_ids:
            all_ops.append(self.cell_ids)
        if self.variable_operations.variables:
            all_ops.append(self.variable_operations)
        if self.variable_values:
            all_ops.append(
                VariableValues(variables=list(self.variable_values.values()))
            )
        if self.datasets.tables:
            all_ops.append(self.datasets)
        all_ops.extend(self.cell_operations.values())
        if self.stale_code:
            all_ops.append(self.stale_code)
+     # need to add append self.datasets
        return all_ops

i just quickly added this copying what i did for DataSourceConnections, which may not have been the correct API. so you may change the Datasets class if you need. (i.e. clear_channel might be useless)

marimo/_sql/engines.py Outdated Show resolved Hide resolved
marimo/_sql/engines.py Outdated Show resolved Hide resolved
@@ -112,6 +112,10 @@ export const DataSourcesPanel: React.FC = () => {
case "duckdb":
code = `_df = mo.sql(f"SELECT * FROM ${table.name} LIMIT 100")`;
break;
case "connection":
// TODO: Handle connection source
code = `_df = mo.sql("SELECT 1", engine=${table.source_type})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be

code = `_df = mo.sql(f"SELECT * FROM ${table.name} LIMIT 100", engine=${table.source_type})`;

I guess none will appear in that sidebar for now? so can wait

@mscolnick
Copy link
Contributor

@Light2Dark, this is awesome! i am going to merge since this is in a good state and we can keep branching off the main branch

@mscolnick mscolnick merged commit 95d3133 into marimo-team:ms/sql-engines Jan 30, 2025
28 of 31 checks passed
Light2Dark added a commit that referenced this pull request Jan 31, 2025
<!--
Provide a concise summary of what this pull request is addressing.

If this PR fixes any issues, list them here by number (e.g., Fixes
-->
I branched out to experiment. `mo.sql` can execute most SQL statements.
Added some frontend.

Todo:
- [ ] no longer existing connections aren't being deleted
- [ ] update selection input display, icons etc (see @/mscolnick's
comment below) **[might not do this here]**
- [ ] fix/add some tests
- [ ] add some docs on how to use this
- [ ] sql regex doesn't handle different ordering of engine & output
params **[probs won't do it now too]**

future work:
- handle data source panel, sql parsing, so that dataset info is
obtained from custom sql engines

<!--
Detail the specific changes made in this pull request. Explain the
problem addressed and how it was resolved. If applicable, provide before
and after comparisons, screenshots, or any relevant details to help
reviewers understand the changes easily.
-->
- by default, frontend will set duckdb as engine (this is hardcoded in
frontend, not provided by backend)
- added a new atom to store the latestEngineSelected, so it's sort of
"smart" when you create new cells

- [X] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [X] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on
[Discord](https://marimo.io/discord?ref=pr), or the community
[discussions](https://github.com/marimo-team/marimo/discussions) (Please
provide a link if applicable).
- [X] I have added tests for the changes made.
- [X] I have run the code and verified that it works as expected.

<!--
Tag potential reviewers from the community or maintainers who might be
interested in reviewing this pull request.

Your PR will be reviewed more quickly if you can figure out the right
person to tag with @ -->

@akshayka OR @mscolnick
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants