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

Hand-wrap TableReplayer/DynamicTableWriter/TableLoggers, fixes #1944 #1949

Merged
merged 22 commits into from
Feb 28, 2022

Conversation

jmao-denver
Copy link
Contributor

@jmao-denver jmao-denver commented Feb 8, 2022

  1. fold the TableReplayer and TableWriter into table_factory.py
  2. renamed TableLoggers to perfmon.py
  3. fixed some unclear documentation in the TableLoggers.java

fixes #1944 #2004 #1730

Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

Just reviewed DyamicTableWriter.java.

pyintegration/deephaven2/table_factory.py Show resolved Hide resolved
pyintegration/deephaven2/table_factory.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/table_factory.py Show resolved Hide resolved
pyintegration/deephaven2/table_factory.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Outdated Show resolved Hide resolved
pyintegration/tests/test_table_factory.py Outdated Show resolved Hide resolved
pyintegration/tests/test_table_factory.py Outdated Show resolved Hide resolved
pyintegration/tests/test_table_factory.py Outdated Show resolved Hide resolved
pyintegration/tests/test_table_factory.py Outdated Show resolved Hide resolved
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Only looked at the jpy / c stuff for now, but I don't think it's safe as-is.

py/jpy/src/main/c/jpy_jtype.c Outdated Show resolved Hide resolved
py/jpy/src/main/c/jpy_jtype.c Outdated Show resolved Hide resolved
py/jpy/src/main/c/jpy_jtype.c Outdated Show resolved Hide resolved
py/jpy/src/main/c/jpy_jtype.c Outdated Show resolved Hide resolved
Copy link
Member

@chipkent chipkent left a comment

Choose a reason for hiding this comment

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

This is a partial review.

Also made TableLogger Pep8 compliant and renamed it to perfmon
Given the choice, users will be drawn to use the convenience methods
logRow() in DynamicTableWriter which accept generic java.lang.Object
in the input parameters. So there is a chance the numeric values don't
match the declared Column data type are passed in. As performance isn't
really a big concern here, we can afford to perform conversions and make
it easier for the users than require them to do cast/conversion before
calling the methods.
The PermissiveRowSetter extends RowSetter with an addtional method that
allows unsafe numeric conversion. The setPermissive() is only called by
logRow() convenience overloads which are only used by the Python API.
pyintegration/tests/testbase.py Show resolved Hide resolved
pyintegration/tests/testbase.py Outdated Show resolved Hide resolved
pyintegration/tests/test_table.py Outdated Show resolved Hide resolved
pyintegration/tests/test_replay.py Show resolved Hide resolved
pyintegration/tests/test_replay.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Outdated Show resolved Hide resolved
pyintegration/deephaven2/perfmon.py Show resolved Hide resolved
@deephaven deephaven deleted a comment from devinrsmith Feb 23, 2022
Comment on lines 78 to 76
self._j_replayer.shutdown()
self._hist_tables = []
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the shutdown call still be here, but the self._hist_tables be gone?

@jmao-denver jmao-denver dismissed devinrsmith’s stale review February 28, 2022 17:52

addressed the concerns

@jmao-denver jmao-denver merged commit 5236ebb into deephaven:main Feb 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2022
@jmao-denver jmao-denver deleted the feature-1944 branch February 8, 2023 18:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate the TableManipulation/TableLoggers modules to deephaven v2 (hand-wrapping/refactoring)
4 participants