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

refactor(api): remove schema #10149

Merged
merged 9 commits into from
Sep 20, 2024
Merged

Conversation

ncclementi
Copy link
Contributor

@ncclementi ncclementi commented Sep 17, 2024

BREAKING CHANGE: Removed hierarchical usage of schema.

Ibis uses the following naming conventions:

  • schema: a mapping of column names to datatypes
  • database: a collection of tables
  • catalog: a collection of databases

Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@ncclementi ncclementi changed the title refactor(api): remove schema refactor(api): remove schema Sep 17, 2024
@ncclementi
Copy link
Contributor Author

I'm not sure I understand how the test_signature.py works, because I clearly broke all of them, but is failing due to an XPASS, any help on that front would be appreciated.

@cpcloud
Copy link
Member

cpcloud commented Sep 17, 2024

If something is xpassing, generally the approach is to delete the marker that's causing the problem.

@gforsyth
Copy link
Member

the signature check is to enforce (eventually) that all the backends call all positional arguments the same thing, etc (that's why currently, almost all of them are xfailed)

When you removed the schema kwarg, it made some of the backend-specific table methods match the method definition from BaseBackend.
also, keyword argument names also have to match, unless those arguments are keyword-only arguments

removing entries from those xfails is a good thing, though

@ncclementi ncclementi added the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 17, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 17, 2024
@ncclementi ncclementi added the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 18, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 18, 2024
@gforsyth gforsyth added this to the 10.0 milestone Sep 18, 2024
@gforsyth gforsyth added the breaking change Changes that introduce an API break at any level label Sep 18, 2024
@ncclementi ncclementi added ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI and removed breaking change Changes that introduce an API break at any level labels Sep 18, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 18, 2024
@ncclementi ncclementi added the breaking change Changes that introduce an API break at any level label Sep 18, 2024
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Looks good overall, i think we lost one bit of testing that we shouldn't have.

docs/backends/_utils.py Outdated Show resolved Hide resolved
docs/backends/_utils.py Outdated Show resolved Hide resolved
ibis/backends/duckdb/tests/test_client.py Outdated Show resolved Hide resolved
ibis/backends/duckdb/tests/test_client.py Show resolved Hide resolved
@ncclementi ncclementi added the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 19, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Sep 19, 2024
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Good riddance!

@gforsyth
Copy link
Member

I've got to check to make sure the commit message is going to be formatted correctly, then we can merge

@gforsyth gforsyth merged commit 9642182 into ibis-project:main Sep 20, 2024
89 checks passed
ncclementi added a commit to ncclementi/ibis that referenced this pull request Sep 24, 2024
BREAKING CHANGE: Removed hierarchical usage of schema. 

Ibis uses the following naming conventions:
- schema: a mapping of column names to datatypes
- database: a collection of tables
- catalog: a collection of databases

---------

Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
Co-authored-by: Gil Forsyth <gil@forsyth.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that introduce an API break at any level
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants