Add optional information about originating function call in DataFrameIOLayer by rjzamora · Pull Request #8453 · dask/dask

rjzamora · 2021-12-03T20:05:28Z

This PR does two simple things:

It adds a new (optional) creation_info argument to DataFrameIOLayer, which simply stores the callable (in "func"), positional arguments (in "args"), and key-word arguments (in "kwargs") used to create the original DataFrame collection. The motivation for this PR is to make basic predicate-pushdown optimizations much simpler in both dask.dataframe.optimize, and in dask-sql. In dask-sql, where it should be possible to extract a DNF-formatted filters argument from Calcite, the creation_info attribute should be all that is needed for filters to be pushed into the underlying read_parquet call.
It removes column_projection from DataFrameLayer, since that method only makes sense in DataFrameIOLayer. Note that I would be happy to move this small change into a standalone PR. However, I am including it here since I accidentally used the same branch for the creation_info change :)

…alled on a DataFrameIOLayer

…IOLayer

rjzamora · 2021-12-03T20:11:55Z

dask/dataframe/io/tests/test_parquet.py

+    # However, we can use `creation_info` to regenerate
+    # the same collection with `filters` defined
+    info = ddf2.dask.layers[ddf2._name].creation_info
+    kwargs = info.get("kwargs", {})
+    kwargs["filters"] = filters
+    ddf3 = info["func"](*info.get("args", []), **kwargs)


cc @quasiben @charlesbluca @jdye64

I am hoping that having access to the original function and arguments like this will make predicate-pushdown relatively straightforward once we have extracted the filters.

Unlike column projection, we cannot just modify the io_func object living within the original DataFrameIOLayer, because the filtering happens up front (before the graph is even generated).

gjoseph92

I don't really understand optimization well enough to comment on the overall strategy here. But this seems reasonable enough to me.

gjoseph92 · 2022-01-20T03:22:14Z

dask/layers.py

@@ -345,20 +345,7 @@ def fractional_slice(task, axes):
 class DataFrameLayer(Layer):


Should this class even exist anymore?

At this point, it acts only as a "label" that we could use for complex HLG optimizations (but that we are not actually using at all yet). So, we could certainly remove it, but will need to add something like it for multi-layer column projection and/or multi-layer predicate pushdown.

Do you think this base class will eventually have some things defined on it (for column projection / predicate pushdown)? Or will it always just be an empty sentinel?

As an empty sentinel, it feels a little weird to me. Like I assume it's supposed to indicate that its subclasses all support some type of interface. I think it would be nice for that interface to be defined (even as an abstract method) on the base class, just for readability.

gjoseph92 · 2022-01-20T03:23:57Z

dask/dataframe/optimize.py

+                warnings.warn(
+                    f"{type(new)} return type for project_columns is deprecated. "
+                    f"This method should now return the projected Layer only.",
+                    FutureWarning,


Is going through a deprecation cycle even necessary here?

Probably not - Just makes me nervous to silently allow deprecated behavior. However, we could also just let non-conforming code fail.

I guess it's just a question of whether DataFrameLayer subclasses are a public API right now. Based on the discussion we recently had with @ian-r-rose, I feel like basically the answer is no—people shouldn't be developing against HLG layers yet.

This little deprecation is simple and fine, but it's even simpler to not have to remember to fully remove it at some point. If internal Dask code is the only thing using this interface, seems fine to me.

gjoseph92 · 2022-01-21T19:10:06Z

Responding to @rjzamora's comment on #8487 (comment):

My interest in leaving DataFrameLayer around for now (1), is that I do intend to use it to store necessary information for multi-layer column projection (and evenutally predicate pushdown). For example, we may use that class to define a base required_columns method designed to return the input columns required to produce a specific set of output columns (for that specific Layer). This would be similar to project_columns, but we would be returning a set of column names, rahter than a new Layer.

I would be +0 on still removing DataFrameLayer for now, and adding it back in when we do add these column projection features. Seems like a pretty small diff to make in both cases, but leaves the code in a cleaner state in the interim, so that anyone else reading it who didn't happen to see this conversation on GitHub doesn't have to wonder why that dead code is there.

Regarding (2): I have no problem with removing the deprecation warning and just letting old behavior fail. However, I seem to remember @ian-r-rose telling me that there is at least one DataFrameIOLayer definition in down-stream code.

If there's downstream use, that would be really good to know about. I didn't realize there was any. I'd still be tempted to not do the deprecation cycle though, and just coordinate with that downstream code to make the change?

ian-r-rose · 2022-01-21T19:15:45Z

Regarding (2): I have no problem with removing the deprecation warning and just letting old behavior fail. However, I seem to remember @ian-r-rose telling me that there is at least one DataFrameIOLayer definition in down-stream code.

If there's downstream use, that would be really good to know about. I didn't realize there was any. I'd still be tempted to not do the deprecation cycle though, and just coordinate with that downstream code to make the change?

It's at least used in dask-bigquery and dask-snowflake. I seem to recall at least one other place where there were some experiments with it, but I'm having a hard time coming up with it right now.

ian-r-rose · 2022-01-21T19:18:35Z

Ah! It was with GDAL in dask-geopandas! https://github.com/geopandas/dask-geopandas/pull/123/files

ian-r-rose · 2022-01-21T19:31:27Z

I should note, all of those I listed are using DataFrameIOLayer, so I think you're probably in the clear

rjzamora · 2022-01-21T19:43:25Z

Okay - I removed DataFrameLayer and the FutureWarning in optimize_dataframe_getitem. We can always add back DataFrameLayer when it actually has a purpose, and the project_columns change is very unlikely to cause problems for downstream users of DataFrameIOLayer (because they are most likely to implement project_columns in the IO function wrapper (where behavior has not changed).

gjoseph92

Great!

rjzamora added 2 commits December 3, 2021 09:24

remove project_columns from DataFrameLayer, since it should only be c…

eff4e31

…alled on a DataFrameIOLayer

fix bug in project_columns change, and add creation_info to DataFrame…

ba543b0

…IOLayer

rjzamora added io highlevelgraph Issues relating to HighLevelGraphs. parquet labels Dec 3, 2021

github-actions bot added the dataframe label Dec 3, 2021

rjzamora commented Dec 3, 2021

View reviewed changes

rjzamora mentioned this pull request Dec 14, 2021

Use map_partitions (Blockwise) in to_parquet #8487

Merged

3 tasks

github-actions bot added the needs attention It's been a while since this was pushed on. Needs attention from the owner or a maintainer. label Jan 3, 2022

gjoseph92 reviewed Jan 20, 2022

View reviewed changes

remove DataFrameLayer and project_columns FutureWarning

c62a2cc

gjoseph92 approved these changes Jan 21, 2022

View reviewed changes

rjzamora merged commit 92140f0 into dask:main Jan 22, 2022

rjzamora deleted the manual-parquet-hlg-filters branch January 22, 2022 01:58

rjzamora mentioned this pull request Jan 22, 2022

Move DataFrame ACA aggregations to HLG #8468

Merged

		@@ -345,20 +345,7 @@ def fractional_slice(task, axes):
		class DataFrameLayer(Layer):

Uh oh!

Conversation

rjzamora commented Dec 3, 2021

Uh oh!

rjzamora Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

gjoseph92 left a comment

Choose a reason for hiding this comment

Uh oh!

gjoseph92 Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

rjzamora Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

gjoseph92 Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

gjoseph92 Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

rjzamora Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

gjoseph92 Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

gjoseph92 commented Jan 21, 2022

Uh oh!

ian-r-rose commented Jan 21, 2022

Uh oh!

ian-r-rose commented Jan 21, 2022

Uh oh!

ian-r-rose commented Jan 21, 2022

Uh oh!

rjzamora commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gjoseph92 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjzamora commented Jan 21, 2022 •

edited

Loading