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

feat: add to_date function to StringValue #9030

Merged
merged 15 commits into from
Apr 24, 2024

Conversation

saschahofmann
Copy link
Contributor

@saschahofmann saschahofmann commented Apr 20, 2024

Description of changes

  • Adds to_date to string types that accepts a format string and parses a string to Date type.

  • Uses a cast to timestamp and extract time as a fallback like this

def visit_StringToDate(self, op, *, arg, format_str):
        return self.f.date(self.f.str_to_time(arg, format_str))
  • Implements native functions for bigquery, clickhouse, MySQL, oracle, postgres, and snowflake

Issues closed

Implements half of #8908

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.

@saschahofmann
Copy link
Contributor Author

@gforsyth I am struggling a little with running the tests. I couldn't find much on setting up the database. I can see that in conftest.py some env variable are read so I provided those for my local postgres database via the .env file (on a side note: I found it weird that the .env is committed? )

Now I seem to be missing an extension

psycopg2.errors.UndefinedFile: could not open extension control file "/opt/homebrew/share/postgresql@14/extension/plpython3u.control": No such file or directory

and it doesn't seem trivial to install it. Is there any documentation on setting up the env for running the tests?

@gforsyth
Copy link
Member

Hey @saschahofmann -- rather than using your own instance, I'd recommend installing just and running just up postgres.
That will spin up a postgres container with all the extensions needed and at the port that our pytest setup expects by default.

@gforsyth
Copy link
Member

There are also some instructions here: https://ibis-project.org/contribute/02_workflow#running-the-test-suite

If you're using a mac M2 it's more involved because of the architecture

@saschahofmann
Copy link
Contributor Author

Perfect. No idea why i didnt find that section. Will experiment around and hopefully have something more complete soon.

@saschahofmann
Copy link
Contributor Author

saschahofmann commented Apr 20, 2024

I now implemented the default fallback in sql/compiler.py L804 like this

def visit_StringToDate(self, op, *, arg, format_str):
        return self.f.date(self.f.str_to_time(arg, format_str))

Is there a way to do it using ibis (something like to_timestamp().date(). Also I was thinking whether SQLGlot might have implemented it for its dialect already similar to how its done for str_to_time but I couldn't find it. In the meantime, I am implementing the different cases for the backends separately.

@saschahofmann saschahofmann changed the title WIP: feat: Add to_date function to Strings feat: Add to_date function to Strings Apr 20, 2024
@saschahofmann saschahofmann changed the title feat: Add to_date function to Strings feat: add to_date function to StringValue Apr 20, 2024
@saschahofmann
Copy link
Contributor Author

For postgres, the format for to_timestamp seems to be translated to the postgres native format but its not obvious to me where this happens? I would need to do something similar for to_date

@gforsyth
Copy link
Member

@saschahofmann
Copy link
Contributor Author

Alright. I found that SQLGlot has actually implemented StrToDate so I am just using that. What do I need to do make the doc test pass?

saschahofmann and others added 2 commits April 22, 2024 23:34
Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
@saschahofmann
Copy link
Contributor Author

I think its ready for review !

@gforsyth gforsyth added the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Apr 23, 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 Apr 23, 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.

This looks really nice, @saschahofmann !

I think this patch will get snowflake passing:

index 75bfd7ce6..c0915d059 100644
--- a/ibis/backends/snowflake/compiler.py
+++ b/ibis/backends/snowflake/compiler.py
@@ -78,6 +78,7 @@ class SnowflakeCompiler(SQLGlotCompiler):
         ops.Hash: "hash",
         ops.Median: "median",
         ops.Mode: "mode",
+        ops.StringToDate: "to_date",
         ops.StringToTimestamp: "to_timestamp_tz",
         ops.TimeFromHMS: "time_from_parts",
         ops.TimestampFromYMDHMS: "timestamp_from_parts",

@saschahofmann
Copy link
Contributor Author

Huh, I think I actually had this but removed it because I thought it was in SQLGlot. Will put it back

@gforsyth gforsyth added the ci-run-cloud Add this label to trigger a run of BigQuery, Snowflake, and Databricks backends in CI label Apr 23, 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 Apr 23, 2024
@saschahofmann
Copy link
Contributor Author

I assume that the failing test is unrelated? At least its not clear to me where this would be coming from?

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.

Thanks for putting this together @saschahofmann! The snowpark error is definitely unrelated and might be flaky, so I'm going to merge this and see what happens on CI.

@gforsyth gforsyth merged commit 0701978 into ibis-project:main Apr 24, 2024
87 of 88 checks passed
cpcloud pushed a commit that referenced this pull request Apr 24, 2024
This showed up in #9030 and I wasn't sure if it was flaky, but it looks
like Snowpark can now run Ibis as `caller`, too!
@saschahofmann saschahofmann deleted the feature/to_date branch October 4, 2024 12:13
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.

2 participants