Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

Implement PyHDK QueryExpr, QueryNode and HDK API except data import. #192

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

ienkovich
Copy link
Contributor

No description provided.

Signed-off-by: ienkovich <ilya.enkovich@intel.com>
@property
def size(self):
res = self.c_node.size()
# For scans we hide virtual column
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't follow closely, c_node.size() isn't this a call to C++ API? And if so, doesn't it already contain the logic for subtracting 1 in case of a scan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In C++ we actually do not hide the virtual column in such a manner. So we do all these modifications for virtual columns in Python.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this should be the user's responsibility? I'd think it's a rather specific feature for modin, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, rowid virtual column is useful and is used by Modin. I don't want users to 'see' this column when they ask for node size or use an integer index to choose a column. I wrote some examples and it feels really confusing when this virtual column affects size and indexing (e.g. ht[-1] would give you the last column in most cases but for scans, it would be the rowid column). I believe the current Python implementation is the right way to go (we still can reference rowid column by its name). Probably, we should adjust C++ API implementation to have a match.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think consistent semantics is important. Anyway, it's not related to this PR. LGTM!

@ienkovich ienkovich merged commit 8d76a0d into main Feb 20, 2023
@ienkovich ienkovich deleted the ienkovich/python-api branch February 20, 2023 02:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants