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.

Sign up for GitHub

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

Jump to bottom

refactor(bigquery): remove unnecessary and misspelled bigquery string find implementation #10119

Merged
merged 1 commit into from
Sep 14, 2024
Merged

refactor(bigquery): remove unnecessary and misspelled bigquery string find implementation #10119

merged 1 commit into from
Sep 14, 2024

Conversation

tswast
Copy link
Collaborator

@tswast tswast commented Sep 13, 2024

Typo visti -> visit

Description of changes

Issues closed

gforsyth
gforsyth reviewed Sep 13, 2024
View reviewed changes
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.

wait, what? hmm, I have to take a look at our tests

@gforsyth
Copy link
Member

Thanks for flagging (and fixing) this @tswast ! I'm going to add find to the updated string tests and see what shakes out.

@cpcloud
Copy link
Member

cpcloud commented Sep 13, 2024

I think this means:

  1. We're not testing all the error modes
  2. The base implementation works fine (since that's what'll be called) and we can probably remove the BigQuery-specific implementation?

@tswast tswast mentioned this pull request Sep 13, 2024
Merged
4 tasks
@cpcloud cpcloud added this to the 10.0 milestone Sep 13, 2024
@cpcloud cpcloud added bug Incorrect behavior inside of ibis bigquery The BigQuery backend ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI labels Sep 13, 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 14, 2024
@cpcloud
Copy link
Member

cpcloud commented Sep 14, 2024

I think we can just delete the bigquery implementation since it's actually more limited than the base implementation and the base implementation has been successfully passing for many many CI runs now.

@tswast @cpcloud
chore: remove bq string find impl in favor of more general base sql i…
a478155 
…mplementation

Discovered from typo `visti` -> `visit`
@cpcloud cpcloud changed the title fix(bigquery): use bigquery-specific StringFind compilation refactor(bigquery): remove unnecessary and misspelled bigquery string find implementation Sep 14, 2024
@cpcloud cpcloud merged commit 0558319 into main Sep 14, 2024
74 checks passed
@cpcloud cpcloud deleted the tswast-patch-1 branch September 14, 2024 09:32
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Sep 24, 2024
@tswast @ncclementi
refactor(bigquery): remove unnecessary and misspelled bigquery string…
79c246e 
… find implementation (ibis-project#10119)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gforsyth gforsyth gforsyth left review comments

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
bigquery The BigQuery backend bug Incorrect behavior inside of ibis
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tswast @gforsyth @cpcloud