feat(cli): add metaxy metadata read command - #1109
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
0737140 to
af9c00b
Compare
danielgafni
left a comment
There was a problem hiding this comment.
Thanks!
Overall looking good, I suggested a few improvements.
Also, please improve the tests to assert more about the data - ideally the full dataframe for Parquet (json and csv will probably lose proper datatypes)
…aStore opt, output refactor, assert_frame_equal
danielgafni
left a comment
There was a problem hiding this comment.
Left a bunch of comments
Overall this looks suspiciously AI-ish, please review the output better
There was a problem hiding this comment.
- Minor comments
- Please consolidate
Literal["csv", "json", "parquet", "markdown"]intoWriteFormat: TypeAlias = ...at the top of this module - I think the tests should be refactored:
- pamaterization: instead of defining a single test for every combinations of
formatandoutputparameters, make a single test using@pytest.mark.parametrizefor both parameters. - data: I think it should be possible to make a single shared
@pytest.fixturefor the datafarme used in these tests.
- pamaterization: instead of defining a single test for every combinations of
- When
--queryis used with a non-Ibis store, please let's not use Polars at all. Just useduckdbinstead. We can fail with a clear message ifduckdbis not available. This is because Polars' support for SQL is way behind DuckDB's. - Since 0.1.7 has been released and Metaxy aims to eventually support
Maptypes by default, let's replace allcollect_to_polarswith_collect_to_arrow, and use DuckDB instead of Polars for writes. For Markdown we can use the Markdown extension. Avoid using Polars completely, let's do not have theimport polarsat all. This is because Polars doesn't support theMaptype.
I am documenting references to all of these docs for educational purposes since previously you said you wanted to learn. I assume good faith here so please take your time to go though them and understand what's expected without AI assistance :) As you can see AI is struggling to write clean (or even correct at all) code without proper human guidance and is useless without it.
Thanks for the docs really appreciate it. I’ll go through them and make sure I understand everything before updating the PR. I’m trying to keep AI usage minimal. Earlier I got stuck while updating the PR and used Antigravity, which may have unintentionally changed some code. I’ll review and fix it properly. |
…ed read tests, DuckDB arrow)
metaxy metadata read command
|
@AnkitSharma-29 fyi once #1130 lands you'll need to use it as |
…o Polars, parameterized tests)
…before calling collect_to_arrow under DuckDB mode
The MetadataStore refactor on main moved metaxy.metadata_store.ibis to metaxy.ext.ibis.metadata_store. Update the metadata read command's import accordingly to resolve the typecheck failure.
danielgafni
left a comment
There was a problem hiding this comment.
Thanks, two small comments left!
| except Exception: | ||
| # Fallback: use DuckDB's built-in display | ||
| md = str(con.query("SELECT * FROM _output")) |
There was a problem hiding this comment.
I think this should just fail with an error. The current fallback doesn't produce Markdown while the user has clearly requested it.
There was a problem hiding this comment.
Good catch....removed the fallback entirely so it no longer emits non-markdown output. While fixing it I found the install_extension("markdown") call was the actual cause of the fallback being hit, and it's unnecessary since markdown is rendered via pandas to_markdown(). It now produces real Markdown and fails loudly if rendering can't happen. Added a test covering the failure case.
There was a problem hiding this comment.
Please do not use Pandas. Just fix the markdown dudkcb extension installation instead (if that's a thing). Avoid Pandas at all cost.
There was a problem hiding this comment.
Removed pandas from the markdown output it's now generated natively from the DuckDB Arrow result, no to_markdown(). There's no installable DuckDB markdown extension, so the table is built inline. pushed in ddff9f4.
Thanks! Both addressed in 261bec5. |
Build the markdown table directly from the DuckDB Arrow result instead of pandas to_markdown(), per maintainer feedback to avoid pandas.
This PR implements the
metadata readcommand as requested in #706. It allows users to quickly execute arbitrary SQL queries against feature metadata using DuckDB and Polars. The command supports a simplified interface with--filterand--selectarguments, as well as an advanced--queryargument. Output can be directed tostdoutor a file inmarkdown,json,csv, orparquetformats. Comprehensive unit tests have been added intests/cli/test_cli_metadata_read.pyto ensure all functionality works as expected.