-
Notifications
You must be signed in to change notification settings - Fork 301
feat: add %%sql --limit and --no-display support #1883
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
Open
djanand
wants to merge
4
commits into
apache:main
Choose a base branch
from
djanand:feat/add-limit-no-display-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3f8b09e
feat: add %%sql --limit and --no-display support
djanand bf30f29
rebase + --limit option to jupyter cell
djanand 45eeeea
store ipython constant flags separately
djanand 16c46d4
address review comments: push limit clause to execution layer and not…
djanand File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,8 +108,24 @@ def decorator(func): | |
| return decorator | ||
|
|
||
|
|
||
| from datafusion.dataframe_formatter import ( | ||
| configure_formatter, | ||
| get_formatter, | ||
| set_formatter, | ||
| ) | ||
|
|
||
| from .extension import BallistaSessionContext, DistributedDataFrame | ||
|
|
||
| # Flags accepted by the ``%%sql`` cell magic, kept as constants so the parser | ||
| # and its error messages share a single source of truth. | ||
| LIMIT_FLAG = "--limit" | ||
| NO_DISPLAY_FLAG = "--no-display" | ||
|
|
||
| # Default number of rows rendered for a ``%%sql`` cell when ``--limit`` is not | ||
| # given. This caps only the display (via datafusion's HTML formatter); the | ||
| # underlying result keeps all of its rows. | ||
| DEFAULT_DISPLAY_LIMIT = 50 | ||
|
|
||
|
|
||
| class BallistaConnectionError(Exception): | ||
| """Raised when not connected to a Ballista cluster.""" | ||
|
|
@@ -221,6 +237,49 @@ def register(self, line: str) -> Optional[str]: | |
| "Currently not supporting the inserted file format" | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _parse_cell_magic_args(line: str): | ||
| """Parse the argument line of a ``%%sql`` cell magic. | ||
|
|
||
| Recognises the ``--no-display`` and ``--limit N`` flags (space form, | ||
| e.g. ``--limit 5``, consistent with the other magics in this module). | ||
| The first non-flag token, if any, is treated as the variable name to | ||
| store the result in. | ||
|
|
||
| Returns a ``(var_name, no_display, limit)`` tuple where ``limit`` is | ||
| ``None`` when ``--limit`` was not supplied. Raises ``ValueError`` for a | ||
| missing or invalid ``--limit`` value. | ||
| """ | ||
| tokens = line.strip().split() | ||
| var_name = None | ||
| no_display = False | ||
| limit = None | ||
|
|
||
| i = 0 | ||
| while i < len(tokens): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be a good practice to store constant ipython flags separately?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure makes sense. addressed |
||
| token = tokens[i] | ||
| if token == NO_DISPLAY_FLAG: | ||
| no_display = True | ||
| elif token == LIMIT_FLAG: | ||
| i += 1 | ||
| if i >= len(tokens): | ||
| raise ValueError( | ||
| f"{LIMIT_FLAG} requires a number, e.g. {LIMIT_FLAG} 5" | ||
| ) | ||
| try: | ||
| limit = int(tokens[i]) | ||
| except ValueError: | ||
| raise ValueError( | ||
| f"{LIMIT_FLAG} expects an integer, got '{tokens[i]}'" | ||
| ) | ||
| if limit < 1: | ||
| raise ValueError(f"{LIMIT_FLAG} must be a positive integer") | ||
| elif not token.startswith("--") and var_name is None: | ||
| var_name = token | ||
| i += 1 | ||
|
|
||
| return var_name, no_display, limit | ||
|
|
||
| @line_cell_magic | ||
| def sql(self, line: str, cell=None) -> Optional[DistributedDataFrame]: | ||
| """ | ||
|
|
@@ -243,28 +302,58 @@ def sql(self, line: str, cell=None) -> Optional[DistributedDataFrame]: | |
| LIMIT 5 | ||
|
|
||
| `my_result` will store the result of the SQL-query | ||
|
|
||
| The cell magic accepts two optional flags before the variable name: | ||
| --limit N Render at most N rows in the cell output (default | ||
| 50). This caps the display only; the stored result | ||
| keeps every row. | ||
| --no-display Run the query and store the result without | ||
| displaying it. | ||
| """ | ||
| if not cell: | ||
| return self._execute_sql(line.strip()) if line.strip() else None | ||
| else: | ||
| var_name = None | ||
| query = cell.strip() | ||
| if not query: | ||
| return None | ||
|
|
||
| args = line.strip().split() | ||
| i = 0 | ||
| while i < len(args): | ||
| if not args[i].startswith("--"): | ||
| var_name = args[i] | ||
| i += 1 | ||
| try: | ||
| var_name, no_display, limit = self._parse_cell_magic_args(line) | ||
| except ValueError as e: | ||
| return str(e) | ||
|
|
||
| result = self._execute_sql(query) | ||
|
|
||
| # Store in user namespace if variable name provided | ||
| # The stored variable always holds the full, untruncated result. | ||
| if var_name and self.shell is not None: | ||
| self.shell.user_ns[var_name] = result | ||
| return result | ||
|
|
||
| if no_display: | ||
| return None | ||
|
|
||
| # Outside IPython there is no auto-render to cap or suppress, so | ||
| # just return the result rather than swallowing it. | ||
| if not IPYTHON_AVAILABLE: | ||
| return result | ||
|
|
||
| # Display-only cap: limits the rows rendered for THIS cell, never | ||
| # the underlying data, so an in-query LIMIT always takes effect. | ||
| # datafusion's formatter is a process-global singleton, so we | ||
| # render eagerly with the cap applied and then restore the previous | ||
| # formatter, keeping the effect scoped to this cell instead of | ||
| # leaking into the rest of the session. Both min_rows and max_rows | ||
| # are set because the formatter requires min_rows <= max_rows. | ||
| rows = limit if limit is not None else DEFAULT_DISPLAY_LIMIT | ||
| previous_formatter = get_formatter() | ||
| try: | ||
| configure_formatter(max_rows=rows, min_rows=rows) | ||
| display(result) | ||
| finally: | ||
| set_formatter(previous_formatter) | ||
|
|
||
| # Returning None avoids a second, un-capped auto-render by IPython; | ||
| # the result is still available via the variable and _last_result. | ||
| return None | ||
|
|
||
| def _connect(self, address: str) -> Optional[str]: | ||
| """Connect to a Ballista cluster.""" | ||
|
|
@@ -487,7 +576,7 @@ def _show_help(self) -> Optional[str]: | |
|
|
||
| %%sql [options] [var] - Execute multi-line SQL query | ||
| Options: | ||
| --no-display - Don't display results | ||
| --no-display - Run the query and store the result without displaying it | ||
| --limit N - Limit displayed rows (default: 50) | ||
| var - Store result in variable | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could we probably push this as a limit option to datafusion ? That might help with probing high volume data
Uh oh!
There was an error while loading. Please reload this page.
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.
The
--limitis a display cap atm. It only changes how many rows render. It does not reduce compute or collection i.e the full result is still materialized. On datafusion side it seems we already havedf.limit(n, offset), df.head(n)which pushes the limit into the plan. I think you mean that setting%%sql --limit xshould push it to the execution layer rather than just display side. right ? I can look into that side lmkThere 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.
Yeah. I personally think that without that wiring in place , this feature might still OOM and users might not have an idea re the root cause