Skip to content

test: pin SQL surface for curate and document exactly one SELECT rule - #285

Open
Prateekathub wants to merge 2 commits into
Hebbian-Robotics:mainfrom
Prateekathub:pin-curate-sql-surface
Open

test: pin SQL surface for curate and document exactly one SELECT rule#285
Prateekathub wants to merge 2 commits into
Hebbian-Robotics:mainfrom
Prateekathub:pin-curate-sql-surface

Conversation

@Prateekathub

Copy link
Copy Markdown

Summary

Closes #279
Added the required documentation to CATALOG.md pinning the exactly-one-SELECT rule.
Added parameterized tests for PIVOT, DESCRIBE, and SUMMARIZE. I have decided to officially support DESCRIBE and SUMMARIZE since DuckDB parses them as SELECT, so they are marked as allowed in the tests.

Why

To fulfill the documentation and testing requirements outlined in issue #279 and ensure future developers don't accidentally remove the security guard.

Validation

Ran uv run ruff check --fix and uv run ruff format successfully.
Local pytest execution failed with ModuleNotFoundError: No module named 'fcntl' because I am on a Windows machine, so I am relying on the GitHub Actions CI to verify the tests run successfully on Linux.

Checklist

  • I added or updated outcome-focused tests for changed business logic.
  • I updated documentation for changed behavior, flags, formats, or requirements.
  • I ran uv run ruff check --fix, uv run ruff format, and uv run ty check.
  • I ran the relevant pytest suite. (Skipped due to Windows fcntl error)
  • I did not add recordings, generated media, credentials, private URLs, or runtime artifacts.
  • I preserved stored-data compatibility or documented an explicit version change.

@github-actions

Copy link
Copy Markdown

👋 Hi @Prateekathub — thank you so much for your first contribution to HFlow!

A maintainer will review your pull request as soon as possible. In the meantime:

💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game.

We are excited to have you here and appreciate your help making the project better! 🙌

@kstonekuan kstonekuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for stating the DESCRIBE / SUMMARIZE decision in the description instead of leaving me to infer it. I disagree with it though.

DESCRIBE and SUMMARIZE pass the guard, but they don't produce a manifest. A curation manifest is a set of episodes. Those two produce a description of columns. What each one actually writes, on current main:

SELECT     rows=1  manifest columns=['episode_id']
DESCRIBE   rows=1  manifest columns=['column_name', 'column_type', 'null', 'key', 'default', 'extra']
SUMMARIZE  rows=1  manifest columns=['column_name', 'column_type', 'min', 'max', 'approx_unique', ...]

and handing each to the next step:

SELECT     -> snapshot OK
DESCRIBE   -> ValueError: snapshot manifest ... must contain an episode_id column

So marking them supported means curate() writes a file it calls a manifest that every consumer refuses. "They parse as SELECT so we support them" is a fact about duckdb's parser, not a decision about what curation means. The issue offered the other option for this reason: pin that they're incidental and not promised. Take that unless you can show a use for the file they produce.

Two of the six queries are unpinned. The issue's table had six rows. TABLE episodes and VALUES (1), (2) are missing, and both work today. The point is that the accepted surface is written down, so leaving two out leaves the same gap in a smaller place.

The docs line needs another pass. It reads:

The curate() function accepts exactly one SELECT statement

No full stop, and it's dropped into the paragraph about passing --sql-file to the CLI, where the surrounding text is about command-line arguments. It also says curate(), the Python function, in a CLI section. Say the rule once, in prose that matches its neighbours, where someone learning to write curation SQL will be looking.

One structural thing. You appended these to _MULTI_STATEMENT_PAYLOADS, which drives test_stage_manifest_and_count_rejects_non_single_select. Neither name describes a PIVOT or an accepted DESCRIBE. That list already carried one row it didn't describe, now it's four. Consider whether the surface table wants to be its own thing.

On validation: the suite passes on Linux, I ran it, so nothing here is a test failure. CI doesn't run automatically for first-time contributors, it waits on approval, so "relying on CI" usually means relying on me. hflow.storage needs fcntl and will never import on native Windows. WSL2 is the way in and several people here work that way.

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.

The SQL surface curate() accepts changed with #271 and is neither documented nor pinned

2 participants