Skip to content

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
work_objs = []
# 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?

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.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