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

Allow to mixing series and expressions in withColumns #58

Merged

Conversation

sezanzeb
Copy link
Contributor

@sezanzeb sezanzeb commented Apr 4, 2023

All tests are passing.

Also tested it on my own, and it worked flawlessly:

	const df = pl.DataFrame(
		{
			"foo": [1, 2, 5],
			"bar": [6, 7, 8],
			"ham": ["a", "b", "c"]
		}
	).lazy()

	console.log(await df.withColumns(
		pl.col('foo').alias('foo2'),
		pl.Series('qux', [1, 2, 3]),
		pl.col('bar').alias('bar2'),
	).collect())
	const df = pl.DataFrame(
		{
			"foo": [1, 2, 5],
			"bar": [6, 7, 8],
			"ham": ["a", "b", "c"]
		}
	)

	console.log(df.withColumns(
		pl.col('foo').alias('foo2'),
		pl.Series('qux', [1, 2, 3]),
		pl.col('bar').alias('bar2'),
	))
shape: (3, 6)
┌─────┬─────┬─────┬─────┬──────┬──────┐
│ foo ┆ bar ┆ ham ┆ qux ┆ foo2 ┆ bar2 │
│ --- ┆ --- ┆ --- ┆ --- ┆ ---  ┆ ---  │
│ f64 ┆ f64 ┆ str ┆ f64 ┆ f64  ┆ f64  │
╞═════╪═════╪═════╪═════╪══════╪══════╡
│ 1.0 ┆ 6.0 ┆ a   ┆ 1.0 ┆ 1.0  ┆ 6.0  │
├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌┤
│ 2.0 ┆ 7.0 ┆ b   ┆ 2.0 ┆ 2.0  ┆ 7.0  │
├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌┼╌╌╌╌╌╌┤
│ 5.0 ┆ 8.0 ┆ c   ┆ 3.0 ┆ 5.0  ┆ 8.0  │
└─────┴─────┴─────┴─────┴──────┴──────┘

There are more places with similar usage of the spread operator (using the search regex , \.\.\. finds them), and isExprArray and isDataFrameArray could be changed as well to use every(...), but I wanted to avoid possibly breaking stuff that I haven't used yet.

@sezanzeb sezanzeb changed the title Allow to mixing series and expressions in withColumns Allow to mixing series and expressions in withColumns Apr 4, 2023
@sezanzeb
Copy link
Contributor Author

sezanzeb commented Apr 5, 2023

Empty withColumns call works fine:

const df = pl.DataFrame(
	{
		"foo": [1, 2, 5],
		"bar": [6, 7, 8],
		"ham": ["a", "b", "c"]
	}
)

console.log(df.withColumns())
shape: (3, 3)
┌─────┬─────┬─────┐
│ foo ┆ bar ┆ ham │
│ --- ┆ --- ┆ --- │
│ f64 ┆ f64 ┆ str │
╞═════╪═════╪═════╡
│ 1.0 ┆ 6.0 ┆ a   │
├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┤
│ 2.0 ┆ 7.0 ┆ b   │
├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌┤
│ 5.0 ┆ 8.0 ┆ c   │
└─────┴─────┴─────┘

@universalmind303 universalmind303 merged commit acd62be into pola-rs:main Apr 5, 2023
@@ -12,6 +12,7 @@ import {
import { _LazyGroupBy, LazyGroupBy } from "./groupby";
import { Deserialize, GroupByOps, Serialize } from "../shared_traits";
import { LazyOptions, LazyJoinOptions } from "../types";
import { Series } from "@polars/series";
Copy link

Choose a reason for hiding this comment

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

A heads up that this absolute import is causing my downstream CI checks to fail on the upgrade PR for the latest version of nodejs-polars because typescript transpilation is failing:

Cannot find module '@polars/series' or its corresponding type declarations.

I can probably tweak my tsconfig to work around this, but thought it was worth flagging since it's inconsistent with the rest of the imports. Maybe something you want to codify for the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into it on monday

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#76

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.

3 participants