-
Notifications
You must be signed in to change notification settings - Fork 653
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-#4244: Implement dataframe exchange protocol for OmniSci #4269
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4269 +/- ##
===========================================
- Coverage 84.82% 60.23% -24.60%
===========================================
Files 213 218 +5
Lines 17321 17841 +520
===========================================
- Hits 14693 10747 -3946
- Misses 2628 7094 +4466
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
…niSci Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
fb32aeb
to
aa1cfc5
Compare
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
modin/experimental/core/storage_formats/omnisci/query_compiler.py
Outdated
Show resolved
Hide resolved
...core/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/utils.py
Show resolved
Hide resolved
...core/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/utils.py
Outdated
Show resolved
Hide resolved
elif pa.types.is_date(dtype): | ||
return getattr(ArrowCTypes, f"DATE{dtype.bit_width}", "DATE64") | ||
elif pa.types.is_time(dtype): | ||
# TODO: for some reason `time32` type doesn't have a `unit` attribute, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ask arrow guys on this?
...ore/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
...ore/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/column.py
Outdated
Show resolved
Hide resolved
...ore/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/buffer.py
Outdated
Show resolved
Hide resolved
...ore/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/buffer.py
Outdated
Show resolved
Hide resolved
.../experimental/core/execution/native/implementations/omnisci_on_native/dataframe/dataframe.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a release note please.
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/test/exchange/dataframe_protocol/omnisci/test_protocol.py
Outdated
Show resolved
Hide resolved
modin/test/exchange/dataframe_protocol/omnisci/test_protocol.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dchigarev, LGTM, thanks!
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
.../execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/dataframe.py
Outdated
Show resolved
Hide resolved
.../execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/dataframe.py
Outdated
Show resolved
Hide resolved
...core/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/utils.py
Outdated
Show resolved
Hide resolved
modin/test/exchange/dataframe_protocol/omnisci/test_protocol.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
This pull request introduces 1 alert when merging 718414d into 5440f08 - view on LGTM.com new alerts:
|
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Outdated
Show resolved
Hide resolved
modin/core/dataframe/pandas/exchange/dataframe_protocol/from_dataframe.py
Show resolved
Hide resolved
...core/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/utils.py
Outdated
Show resolved
Hide resolved
modin/test/exchange/dataframe_protocol/omnisci/test_protocol.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there :)
Please also answer #4269 (comment)
.../execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/dataframe.py
Outdated
Show resolved
Hide resolved
...core/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/utils.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
...ore/execution/native/implementations/omnisci_on_native/exchange/dataframe_protocol/column.py
Show resolved
Hide resolved
Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Dmitry Chigarev dmitry.chigarev@intel.com
What do these changes do?
OmnisciOnNativeDataframe
:from_dataframe
function, converting DataFrame exchange object to pandas DataFrame.modin/test/exchange/dataframe_protocol/omnisci
).OmnisciOnNative.from_dataframe
in order to satisfy the protocol's requirement. Link to the discussion about this in our PR.flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date