Skip to content

Issue139/monetdbe append #167

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

Merged
merged 25 commits into from
Jan 14, 2022
Merged

Issue139/monetdbe append #167

merged 25 commits into from
Jan 14, 2022

Conversation

aris-koning
Copy link
Contributor

pull request for new numpy/type features

@aris-koning aris-koning linked an issue Jan 5, 2022 that may be closed by this pull request
# cffi_objects assists to keep all in-memory native data structure alive during the execution of this call
cffi_objects = []
cffi_objects = list()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think in an other PR or a previous commit i merged work_objs and cffi_objects, since afaik they do the same thing, or not?

@@ -48,34 +48,33 @@ class MonetdbTypeInfo(NamedTuple):
numpy_type: np.dtype
c_string_type: str
py_converter: Optional[Callable]
null_value: Optional[Union[int, np.floating]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you removed that to fix mypy complaining, numpy added new type annotations, and in master i fixed the type annotation for the null field (an optional np.floatable I believe)

np_col: np.ndarray = np.array([extract(rcol, r) for r in range(result.nrows)])
values = [extract(rcol, r) for r in range(result.nrows)]
np_col: np.ndarray = np.array(values)
np_mask = np.array([v is None for v in values])
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, this seems almost the same except for an extra list comprehension.
mask = np_col == type_info.null_value is evaluated in numpy land and should be faster

my guess is you did this to get rid of the monetdbType null value field, see comment above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

added an issue for this #168


work_column.type = type_info.c_type
work_column.count = column_values.shape[0]
work_column.name = ffi.new('char[]', column_name.encode())
if type_info.numpy_type.kind == 'U':
if type_info.numpy_type.kind == 'M':
t = ffi.new('monetdbe_data_timestamp[]', work_column.count)
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a comment, no action required, but this function is getting a bit messy, we should split it up in the future.

np_col: np.ndarray = np.array([extract(rcol, r) for r in range(result.nrows)])
values = [extract(rcol, r) for r in range(result.nrows)]
np_col: np.ndarray = np.array(values)
np_mask = np.array([v is None for v in values])
Copy link
Collaborator

Choose a reason for hiding this comment

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

added an issue for this #168

@gijzelaerr gijzelaerr merged commit ac23823 into master Jan 14, 2022
@gijzelaerr gijzelaerr deleted the issue139/monetdbe_append branch January 14, 2022 11:32
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.

monetdbe append handles all types (currently no conversion)
2 participants