FEAT: Create ExtractWeekOfYear operation #2177

xmnlab · 2020-04-04T20:32:35Z

Create ExtractWeekOfYear operation and add its support to Clickhouse, CSV, MySQL, Pandas, Parquet, PostgreSQL, PySpark, SQLite and Spark

notes:

sqlite needed some custom function using example from https://stackoverflow.com/a/15511864/3715476
omniscidb needed some custom function using algorithms explanation from https://en.wikipedia.org/wiki/ISO_week_date

xmnlab · 2020-05-30T02:39:00Z

this PR is ready for review! thanks!

xmnlab · 2020-06-12T22:57:10Z

ibis-project.ibis (LinuxTest py37_sql_parquet) tests worked, but Publish Linux environment YAML to Azure give an error:

Information, ApplicationInsightsTelemetrySender correlated 2 events with X-TFS-Session 33917c70-b933-4976-8df8-aa9153851b7f
##[error]Artifact LinuxCondaEnvironment-37-main already exists for build 2595.
Finishing: Publish Linux environment YAML to Azure

probably it is not a block for review or merge.

it is ready again for review. thanks.

datapythonista

looks good, added some comments to make the code more clear

datapythonista · 2020-06-19T13:23:18Z

ibis/omniscidb/operations.py

+    https://en.wikipedia.org/wiki/ISO_week_date
+    """
+    # n_weeks_year adjustment
+    def _p(year):


can you replace _p by a descriptive name? Also, the heading _ is not needed, since this function it's inside another, so it doesn't add much value to mark it as private (it can't be used anywhere else anyway).

I used p here because it is the name of the function used by the algorithm presented in the link in See Also section.
but I can change that.

datapythonista · 2020-06-19T13:25:18Z

ibis/omniscidb/operations.py

+    dow = d.day_of_week.index() + 1
+
+    result = doy - dow
+    result += 10


I think it makes the code less readable to have all this, instead of simply:

d.day_of_year() - d.day_of_week.index() + 11

you're right ... I just follow the algorithm from https://en.wikipedia.org/wiki/ISO_week_date .. I forgot to refactor that. thanks!

datapythonista · 2020-06-19T13:27:15Z

ibis/omniscidb/operations.py

+        )
+
+    op = expr.op()
+    return translator.translate(_extract_woy_expr(op.args[0]))


Why do we need a nested function here?

Also, things like op = expr.op() seem unnecessary and confusing, if you are going to use the result just once.

you're right .. probably I needed that for some reason in the past .. or something like that .. and forgot to refactor that. thanks!

datapythonista · 2020-06-19T13:28:38Z

ibis/sql/sqlite/compiler.py

+        )
+        / 7
+        + 1,
+        sa.INTEGER,


I guess it's black who is creating this unreadable code. But you can use a variable here instead.

ok I will improve that. thanks!

xmnlab

thanks for the feedback @datapythonista
I will work on that.

xmnlab · 2020-06-20T21:46:28Z

ibis/omniscidb/operations.py

+    https://en.wikipedia.org/wiki/ISO_week_date
+    """
+    # n_weeks_year adjustment
+    def _p(year):


I used p here because it is the name of the function used by the algorithm presented in the link in See Also section.
but I can change that.

xmnlab · 2020-06-20T21:48:13Z

ibis/omniscidb/operations.py

+    dow = d.day_of_week.index() + 1
+
+    result = doy - dow
+    result += 10


you're right ... I just follow the algorithm from https://en.wikipedia.org/wiki/ISO_week_date .. I forgot to refactor that. thanks!

xmnlab · 2020-06-20T21:53:02Z

ibis/omniscidb/operations.py

+        )
+
+    op = expr.op()
+    return translator.translate(_extract_woy_expr(op.args[0]))


you're right .. probably I needed that for some reason in the past .. or something like that .. and forgot to refactor that. thanks!

xmnlab · 2020-06-20T21:53:58Z

ibis/sql/sqlite/compiler.py

+        )
+        / 7
+        + 1,
+        sa.INTEGER,


ok I will improve that. thanks!

datapythonista

Nice changes, added some comments that I think should improve readability

datapythonista · 2020-06-21T11:54:30Z

ibis/omniscidb/operations.py

+    https://en.wikipedia.org/wiki/ISO_week_date
+
+    """
+    # adjustment factor for week year extraction


Do you mind moving this to the docstring of adjustment factor and provide a proper docstring for it? For me, and I guess for anyone else reading the code, it's not obvious what this is doing. Would be nice to have more information, including examples. I guess this is related to leap-years, but would be nice to know exactly what's the reasoning here, without having to spend a decent amount of time reading the code and researching.

Can you remove the line # adjustment factor for week year extraction please. I think it was left here unintentionally.

datapythonista · 2020-06-21T11:54:43Z

ibis/omniscidb/operations.py

+    def adjustment_factor(year):
+        return (year + (year // 4) - (year // 100) + (year // 400)) % 7
+
+    need_adjustment = (adjustment_factor(year) == 4) | (


Suggested change

need_adjustment = (adjustment_factor(year) == 4) | (

needs_adjustment = (adjustment_factor(year) == 4) | (

datapythonista · 2020-06-21T11:55:24Z

ibis/omniscidb/operations.py

+
+def _woy_preliminary(d: Union[ir.DateValue, ir.TimestampValue]) -> ir.Expr:
+    """
+    Return a preliminary week of year (WOY).


Can you explain in another paragraph what a preliminary week of the year is please

datapythonista · 2020-06-21T11:56:13Z

ibis/omniscidb/operations.py

+
+    Parameters
+    ----------
+    d : ibis.expr.types.DateValue or ibis.expr.types.TimestampValue


What is d? Would be nice to have a more descriptive name, and also a description if the name is not clear enough.

datapythonista · 2020-06-21T11:57:16Z

ibis/omniscidb/operations.py

+    d = expr.op().args[0]
+
+    w = _woy_preliminary(d)
+    y = d.year()


Please, avoid naming variables like d, w, y, and use descriptive names.

datapythonista · 2020-06-21T11:59:35Z

ibis/pyspark/compiler.py

-
-
-@compiles(ops.ExtractQuarter)
-def compile_extract_quarter(t, expr, scope, **kwargs):


Is this intentional? Feels unrelated

you're right, maybe some problem after rebasing ... thanks!

datapythonista · 2020-06-21T12:02:17Z

ibis/omniscidb/operations.py

+    return 52 + (ibis.case().when(need_adjustment, 1).else_(0).end())
+
+
+def _woy_preliminary(d: Union[ir.DateValue, ir.TimestampValue]) -> ir.Expr:


Is a function really needed for this? This is being used just once, feels like moving this two lines of code inside _extract_woy would keep things much simpler.

datapythonista · 2020-06-21T12:04:07Z

ibis/omniscidb/operations.py

+    return (result // 7).cast('int16')
+
+
+def _extract_woy(translator, expr) -> str:


I think being explicit about names will make things easier. In this PR woy is clear what it means by the context. But when working on something unrelated, going to read the docstring will be needed. While naming it week_of_year will make it immediate to know.

Suggested change

def _extract_woy(translator, expr) -> str:

def _extract_week_of_year(translator, expr) -> str:

datapythonista

Looks great. Added couple of small comments, to lgtm.

datapythonista · 2020-06-25T10:11:23Z

ibis/omniscidb/operations.py

+    """
+    date = expr.op().args[0]
+
+    week = _week_of_year_preliminary(date)


Creating a separate function for _week_of_year_preliminary seems unnecessary and overcomplicated:

Suggested change

week = _week_of_year_preliminary(date)

week_of_year_without_adjustment = ((date.day_of_year() - date.day_of_week.index() + 11) // 7).cast('int16')

thanks @datapythonista I did this change. just keep the variable name to week and added a comment about this there.

ibis/omniscidb/operations.py

datapythonista · 2020-06-25T10:15:51Z

ibis/omniscidb/operations.py

+    https://en.wikipedia.org/wiki/ISO_week_date
+
+    """
+    # adjustment factor for week year extraction


Can you remove the line # adjustment factor for week year extraction please. I think it was left here unintentionally.

xmnlab · 2020-06-25T15:34:43Z

@datapythonista the comment # adjustment factor for week year extraction was to skip the follow docstring checking error:
D202: No blank lines allowed after function docstring (found 1)

and black add an empty line automatically before the function definition.

I will replace it by # noqa: D202

datapythonista · 2020-06-26T09:24:29Z

I will replace it by # noqa: D202

Try to keep in mind that eventually someone will read that code, and would wonder what is the reason for it. Adding a comment for the reader, besides the comment for the linter, will make our future lives easier. Something like: # noqa: D202 flake-8 incorrectly fails if there is a function definition immediately after the docstring

jreback · 2020-06-26T20:57:15Z

ibis/omniscidb/operations.py

+        .end()
+    )
+
+    return translator.translate(result)


why are you writing these as ibis operations? if the db doesn't support them, then don't add them.

ok. removed. thanks.

datapythonista · 2020-07-15T19:07:26Z

@xmnlab many files conflicting here, can you rebase please?

xmnlab · 2020-07-15T19:47:04Z

@datapythonista thanks! rebased!

datapythonista · 2020-07-27T14:28:34Z

ibis/sql/sqlite/compiler.py

@@ -144,6 +144,39 @@ def _extract_epoch_seconds(t, expr):
    return sa.cast(sa_expr, sa.BigInteger)


+def _extract_week_of_year(t, expr):


Not sure if I'm missing something, but in this comment Jeff said that if the db doesn't support the operation, let's simply not have it. Shouldn't we be removing this function then?

I can remove it too. I will push the changes in a bit.

datapythonista

Thanks @xmnlab, lgtm

datapythonista · 2020-09-07T12:06:53Z

@jreback I think your comments were addressed. Do you mind having another look here? I think this should be ready.

xmnlab force-pushed the add-extract-week-of-year branch from c5a77f6 to 0cee0aa Compare April 5, 2020 01:45

jreback added the feature Features or general enhancements label Apr 9, 2020

xmnlab force-pushed the add-extract-week-of-year branch 3 times, most recently from b44f888 to 6786c58 Compare May 30, 2020 01:58

xmnlab marked this pull request as ready for review May 30, 2020 02:38

xmnlab force-pushed the add-extract-week-of-year branch 2 times, most recently from c771f92 to 6feeb2f Compare June 12, 2020 19:22

datapythonista reviewed Jun 19, 2020

View reviewed changes

xmnlab commented Jun 20, 2020

View reviewed changes

datapythonista reviewed Jun 21, 2020

View reviewed changes

xmnlab force-pushed the add-extract-week-of-year branch from bf61069 to d255621 Compare June 25, 2020 02:21

datapythonista approved these changes Jun 25, 2020

View reviewed changes

datapythonista assigned jreback Jun 26, 2020

jreback suggested changes Jun 26, 2020

View reviewed changes

xmnlab force-pushed the add-extract-week-of-year branch from 0310253 to 3d09d2f Compare July 6, 2020 01:25

xmnlab force-pushed the add-extract-week-of-year branch from b566abe to ebc168a Compare July 15, 2020 19:46

xmnlab mentioned this pull request Jul 26, 2020

[IBIS] FEAT: Need to add Ibis support for various timestamp component operations in OmniSci Quansight/omnisci#56

Open

xmnlab force-pushed the add-extract-week-of-year branch from ebc168a to 99ad527 Compare July 26, 2020 01:40

datapythonista reviewed Jul 27, 2020

View reviewed changes

datapythonista approved these changes Jul 27, 2020

View reviewed changes

FEAT: Create ExtractWeekOfYear operation

9c63372

xmnlab force-pushed the add-extract-week-of-year branch from 034b95f to 9c63372 Compare August 13, 2020 15:48

xmnlab requested a review from jreback August 13, 2020 15:48

jreback added this to the Next Bugfix Release milestone Sep 7, 2020

jreback approved these changes Sep 7, 2020

View reviewed changes

jreback merged commit a37c24c into ibis-project:master Sep 7, 2020

	need_adjustment = (adjustment_factor(year) == 4) \| (
	needs_adjustment = (adjustment_factor(year) == 4) \| (



		@compiles(ops.ExtractQuarter)
		def compile_extract_quarter(t, expr, scope, **kwargs):

		return 52 + (ibis.case().when(need_adjustment, 1).else_(0).end())


		def _woy_preliminary(d: Union[ir.DateValue, ir.TimestampValue]) -> ir.Expr:

		return (result // 7).cast('int16')


		def _extract_woy(translator, expr) -> str:

	def _extract_woy(translator, expr) -> str:
	def _extract_week_of_year(translator, expr) -> str:

	week = _week_of_year_preliminary(date)
	week_of_year_without_adjustment = ((date.day_of_year() - date.day_of_week.index() + 11) // 7).cast('int16')

		@@ -144,6 +144,39 @@ def _extract_epoch_seconds(t, expr):
		return sa.cast(sa_expr, sa.BigInteger)


		def _extract_week_of_year(t, expr):

FEAT: Create ExtractWeekOfYear operation #2177

FEAT: Create ExtractWeekOfYear operation #2177

Conversation

xmnlab commented Apr 4, 2020 • edited Loading

xmnlab commented May 30, 2020

xmnlab commented Jun 12, 2020

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented Jun 25, 2020 • edited Loading

datapythonista commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Jul 15, 2020

xmnlab commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista commented Sep 7, 2020

xmnlab commented Apr 4, 2020 •

edited

Loading

xmnlab Jun 22, 2020 •

edited

Loading

xmnlab commented Jun 25, 2020 •

edited

Loading