Repository navigation
Sync upstream/main: merge PR 12 (output row shapes and time units, two aborts, a use-after-free, the cache fallback) - #35
Merged
Conversation
A child that failed to convert was wrapped with its field name only on the declared path. With output_schema left as None the same failure named the output column alone, as in "output column 'out': Unsupported numpy type 15", and the field it choked on had to be guessed from the dtype. Both paths now convert a field through one helper that names it, the docstring says so, and the catalogue pins the inferred path.
The names were read from the argument inside the batch loop, so a generator, map() or filter() handed in was used up by the first batch, every later batch was adapted with no columns at all, and the UDF died on a bare KeyError that said nothing about input_columns. The names are read once now, deduplicated in first-seen order as before, the docstring says a one-shot iterable serves as well as a list, and the catalogue pins the read-once line.
… field The earlier commit moved the raise that entry mutated into _record_field, so the entry's old text was gone and the catalogue reported it stale instead of running it. The entry now mutates the helper's raise, which drops the field name on the declared and the inferred path alike, and the tests for both kill it.
…r days, before Arrow reads a datetime64 or timedelta64 output pa.array reads numpy's base unit and ignores a multiplier, so a datetime64[5s] column of five-second bins came back at one-second steps, 2020 read as 1980, and datetime64[2D] slipped past the day-unit inference into the misread it guards against. An hour or minute unit, and a week, month or year unit, raised ArrowNotImplementedError against the docstring's promise of a timestamp of the unit. A multiplier now folds into its base unit, hours and minutes become seconds, weeks, months and years become days and so date32, a unit finer than a nanosecond or a month or year timedelta is refused by name, and the docstring and README say so.
…racter per row
A str or bytes returned as a column went to pa.array, which iterates it, so {'country': 'US'} over a two-row batch was the rows U and S, and a 0-d unicode or bytes array's tolist() is that scalar, which defeated the one-dimensional refusal pa.array gives the array itself. Both are refused naming the column, and the message says how to build a constant column.
A helper's Nullable wrapped once more with the input's bitmap went to pa.array as the 2-tuple it is and came back as a two-row list column, its data as one row and its bitmap's bytes as the other, silently on every batch whose column had no validity buffer and with a message blaming a resize otherwise.
…n, sequences by __getitem__, key/value entry dicts, and refuse a namedtuple in another field order
The key check kept only Mapping rows and 2-tuple map entries, and pa.array accepts more: a struct row given as a tuple, a namedtuple or a pyspark Row binds by position, a sequence by __getitem__ alone iterates without an __iter__ attribute, and a map entry may be a {key, value} dict, the shape Spark's map_entries produces. Behind any of those a mistyped nested key was silently nulled, and a namedtuple or Row naming the declared fields in another order swapped every same-typed field. Tuple rows are now checked by position, iterability is tested with iter, entry dicts are read as pairs, a permuted namedtuple is refused by name, and a pyarrow scalar row is left to pa.array, which from pyarrow 21 made a MapScalar a Mapping whose values is an array and crashed the check.
… KeyError, and combine a chunked array pa.array hands back pa.array reads a pandas Series row by its index labels, so a sorted or filtered one came back reordered without a word and one from a groupby died on a bare KeyError(0); a DataFrame under one output key died the same way or came back transposed; a KeyError raised inside pa.array escaped _to_arrow and _record_field unnamed although renamed has a branch for it; and a Series over a multi-chunk pyarrow array came back from pa.array as a ChunkedArray that RecordBatch.from_arrays refused naming no column. Series and DataFrame rows and a DataFrame column are refused naming the column and the remedy, KeyError joins both except tuples, and a ChunkedArray is combined.
…, a map against a declared list of entries, and the list view layouts The ready-built-array field guard compared like kinds only, so an extension array with struct storage cast to a declared struct, a MapArray cast to a declared list of key/value structs, and a list_view or large_list_view container declared for dict rows all came back all-null with no refusal. An extension type is compared and key-checked through its storage, a map is paired with a list of entries through its entries struct, and the view layouts count as list-like where the installed pyarrow has them.
…st, naming the column Without output_schema every list, tuple or object column's type was inferred from its own batch, so an empty or all-None batch beside a full one, or ints beside floats, carried a second schema into Spark's writer, which refused it with Tried to write record batch with different schema and named nothing. The first batch's schema is now held for the partition and a later batch that differs is refused naming the column, both types and the remedy; the string and bytes pins stand as they were.
_with_validity chose its path by the extension type, which reports no fields and no dictionary whatever its storage, so a Nullable over dictionary storage took the from_buffers path the dictionary term exists to avoid and aborted the interpreter, and one carrying a null went to if_else, which has no extension kernel. The storage is masked and the result rewrapped.
structured_array_adapter called flatten() on every child of a struct with a null row before any child was dispatched, and flatten() hands the struct's validity to each child; a union carries no validity buffer of its own, so Arrow's C++ layer failed its check and aborted the process where the documented NotImplementedError naming the field was due. A union child, extension storage included, is refused by name before anything is flattened.
np.frombuffer handed a memoryview keeps only a wrapper of it as the result's base, and that wrapper's release() drops the memoryview's hold on the source, so a caller who released it, dropped the source array and read the view read freed memory, a segfault at 64 Ki elements, reachable through the public adapters and a UDF's data_dict. The read-only memoryview is now wrapped in a foreign pyarrow buffer, which is what the view keeps as its base: it has no release(), holds the source through the memoryview, and exports read-only, so the flip stays refused. The docs test's view detection accepts a pyarrow buffer at the end of the chain.
…fusal The one-dimensional guard was meant for the unicode and bytes route alone, where tolist() turns a 0-d array into a scalar and a 2-d one into nested lists and so defeats the refusal pa.array gives the array itself; every other dtype gets that refusal, which names the column, as before.
The generator kept the adapted arrays, the views, the struct triples and the handed-out bitmaps bound in its frame while the consumer wrote the batch out and the next one was adapted, so a string column's |U copy was live twice at the peak, the README's 1.6 GB batch peaking at 3.2 GB, and a handed-out bitmap outlived the batch its docstring says it lives for. Those names are cleared before the yield, and the yielded batch is dropped on resume.
numpy names every structured dtype of one itemsize void<bits>, so two of them compiled under one qualname, landed in one cache index with identical argtypes, and a process that loaded both from the cache ran the first one's code for the second, boxing an int32 field's bytes as float32. A digest of the dtype's description joins the qualname of a structured dtype.
The date32, date64 and timestamp handlers took the bitmap from the result of an integer cast, and pyarrow drops the validity buffer when it casts a zero-length array, so a zero-row column that still carried one adapted to a bitmap of None, against the documented rule that None means no buffer, where every other type gives an empty uint8 array. The bitmap comes from the source array when the column is empty.
The struct key check listed every distinct key no declared field has, and that list is sized by the data rather than the schema: a UDF keying a dict by a row value put every key of a 100,000-row batch, 1.5 MB, into the exception and twice into the executor logs. Ten are shown with a count of the rest, as a wide type is cut.
… type The dispatcher's fallback read len() and .type of whatever it was handed: a null list scalar died on len(), a struct or map scalar of a supported column was described as an unsupported array of that type, and a pandas frame or record array with a column called type put that column's values into the message or died in type_repr. A scalar is named as one, and only a DataType reaches the message.
A dict holds one value per name, so a name declared twice at any depth was filled twice from the same entry, and the batch died in the JVM with not all nodes and buffers were consumed, naming neither the column nor the repeat; the input side refuses the same shape by name. The schema is checked once, when the function is made.
The generator read .schema of whatever its iterator yielded, so udf(batch) and udf(table) walked the columns and died on an attribute of the first one, and mapInPandas's pandas frames died the same way. An item that is not a RecordBatch is refused naming the shape mapInArrow passes.
One message served both failures, so a value that was valid JSON but a list, a number, null, true or a string was told it must be valid JSON, and neither failure showed the value; the sibling refusals in the same function name the option and repr the value. Both messages now name the object requirement and the value, and the JSON error names its position.
…d say what the function does cast_64bit_date_arrow_to_numpy_array's docstring offered a cast to various units and its body reinterpreted the int64 payload, so timestamp[ms] asked for as datetime64[s] came back in the year 51971 for a caller who followed the docs page. The unit must be the array's own now, a mismatch is refused naming both, and the docstring says it is a view.
…ze binary type tolist() drops a trailing NUL, so one digest in 256 came back a byte short and the whole batch was refused under fixed_size_binary(16), and the docstring blamed numpy for a byte the buffer still held. Under a fixed-size binary type pa.array keeps every byte of the array itself, and only there; variable-width binary still cuts at the first NUL and keeps the tolist() route.
Two same-typed sibling fields, a map's key and its value, and different nesting depths all raised a byte-identical message naming the column and the declared struct alone. The check carries the path through its recursion, so the refusal reads output column 's': field 'm': map value: declared ...
…a can write none numba sets a cached function up when it is decorated and raises RuntimeError there when no cache location can be written: a read-only install, an unwritable site-packages and user cache directory, or an import from an .egg, .whl or .pyz archive, which Spark's --py-files ships. The import then failed with no locator available, naming neither NUMBA_CACHE_DIR nor NUMBARROW_JIT_OPTIONS. The three compiled functions and the viewers now decorate through one helper that compiles such a function without a cache and warns with both remedies, and the README says how the cache works.
…w to derive output_schema from the Spark schema The positional-binding paragraph covered top-level columns; a record array's dtype order and a dict's key order decided where each struct field landed, batch by batch when some rows left a null field out, and the docstring's remedy did not mention that output_schema binds struct fields by name or that its column order has to match the schema mapInArrow is given, which this function never sees. The docstring and the README say both, and name to_arrow_schema as the way to keep the two orders equal.
… where an unsigned type is declared The closed list omitted a timestamp into a time type, which drops the date, a float into a decimal, which rounds, a number into bool, and a float into a narrower float than float32; and a negative value under an unsigned declared type raises OverflowError, which the documented except pa.ArrowInvalid does not catch, as does a value beyond the type's own ceiling rather than beyond int64.
…ames, and say which route flips a view writable is_null's docstring said every negative index stays in bounds and bounds checking cannot catch it, which holds only down to minus eight times the bitmap's length. Sphinx read the trailing underscore of :param index_: and :param dtype_: as a reference marker and rendered index and dtype, so a keyword call copied from the docs raised TypeError. A slice or reshape of a view that an njit function returns is boxed writable by numba and its flag can be flipped, a route the read-only paragraph did not mention; pyarrow's own view has it too.
…ld itself The test's comment credited the folded bitmap with seeing the null struct row, but Spark's own Arrow writer nulls the child of such a row before the batch arrives, so the test passes with the fold reduced to a no-op; the fold is pinned by the non-Spark tests that build a child carrying no null of its own, and this test is the end-to-end check that the row reaches the UDF at all.
The pyarrow-range cells install no pandas, and the test built its frame with to_pandas() unconditionally, so both cells went red on an import rather than on what the test checks.
A requires-python written as ~=3.12 names the same floor as >=3.12, and the gate refused it; both clauses are read now, and a set with neither still stops the gate with a message naming the field.
The README's cache section named NUMBA_CACHE_DIR and NUMBARROW_JIT_OPTIONS as the remedies without saying when they are read. Set after the import, from inside Python, NUMBA_CACHE_DIR left numba's own setting empty and the index files landed in the package's __pycache__; set before it, they landed under the directory named. The section says so now.
A generator already used up, or a filter that matched nothing, named no column, and the function was made without a word: a main_func reading data_dict["x"] died on a bare KeyError, and one doubling whatever arrived returned a batch of no rows and no columns. An empty selection is refused when the function is made, naming both causes and None as the way to every column; the docstring says so, the test covers a list, a used-up generator and a filter, and the catalogue pins the guard.
…t have The order check compared the two sets of names, so a row with one field misnamed, Point(lon=10, latitude=50) under struct<lat, lon>, passed it and pa.array bound the row by position: the lon value landed in lat and the latitude value in lon, with nothing raised. The guard now refuses any field names that are not the declared ones in the declared order, saying whether the names are permuted or not the declared fields at all, with a test for the misnamed row and one for a row naming an extra field, and a catalogue entry for the new term beside the existing one, which follows the rewritten line.
… list The row check looked only inside a list or a tuple, so the same sorted Series handed over as the one element of an object array reached pa.array and came back in label order, [3, 1, 2] for values stored as [1, 2, 3], with nothing raised. Every shape of column that reaches the check is looked through now, with a test over an object array under a declared list type and under inference, and a catalogue entry that puts the list-or-tuple gate back.
…ruct The refusal of a Series row named row.to_numpy() and list(row) as the remedies, and under a declared struct column pa.array refuses both in turn, wanting a dict or a tuple; the only shape that works there, row.to_dict(), went unnamed. The remedy now follows the declared type, with a test that follows it under a struct and a catalogue entry for the term.
…her than rescaling its counts Taking an hour, minute, week or day unit to seconds or days happened before the declared type was applied, so counts of [1, 2] days declared int64 came back as [86400, 172800] with nothing raised, where pa.array had refused the unit outright. The rescale now waits for a temporal declared type, or none, and under any other type the array is refused naming the unit and the two ways out; a multiplier still folds under every type, since pa.array would otherwise read the count as one of the base unit. The factory docstring says so, a test covers the three units under int64 and string, the fold under int64 and the coarse unit under date32, and a catalogue entry pins the term.
…ily, not by a unit's presence The view refused a type with no unit as not a date64 or timestamp, and a time64 or a duration carries a unit too, so an hour of the day and a ninety-second span were viewed as instants on 1970-01-01. The type family is tested now, with the two shapes in the view's test and a catalogue entry that puts the unit test back.
…es for every dtype The cache-name suffix hashed dtype.descr, which numpy refuses to build for a dtype with out-of-order or overlapping fields, the very dtype its own multi-field indexing, rec[["b", "a"]], hands back, so the factory raised ValueError where it had built a viewer before. The repr tells the dtypes apart as well and exists for all of them; a test builds the viewer over the reordered dtype and reads through it, and a catalogue entry puts descr back.
… test The decorator re-raises any RuntimeError at decoration but numba's "no locator available", and nothing tested that term: with it dropped, every such failure compiled uncached behind a warning about the cache, and the suite stayed green. A test hands the decorator another RuntimeError and expects it back, then the no-locator one and expects the uncached recompile behind the warning; a catalogue entry drops the term.
The check looks inside a map's entries and through an extension type's storage, and only its struct and list paths were exercised: deleting either term left the suite green. The factory-time test now declares the repeated name inside a map and under an extension type, over an extension type the tests' common module defines so that every pyarrow the matrix runs exercises it, and two catalogue entries delete the terms.
The refusal of a union under a struct looks through an extension type over the union, and nothing exercised that: with the unwrap deleted the suite stayed green while flatten() would again hand the struct's validity to a child that cannot take it. The union test wraps its union in an extension type as well, and a catalogue entry deletes the unwrap.
The key check looks through an extension type over a struct twice, when it asks whether a type carries keys and when it reads the fields, and nothing exercised either: with one unwrap deleted the suite stayed green while pa.array built a column of nulls for dicts keyed Amount under an extension type over struct<amount>. The any-depth test gains that shape, and two catalogue entries delete the unwraps one at a time.
…barrow is The README said both cache variables are read when numbarrow is first imported. NUMBARROW_JIT_OPTIONS is; NUMBA_CACHE_DIR is numba's, read when numba is imported, which another library may have done earlier, and again only when numba compiles something, so a directory set between the two imports missed the first function numbarrow compiles, and all of them when the old cache was warm. The paragraph says so now, and a test imports numba under one directory, switches to another and imports numbarrow, expecting the first function's index under the first.
The factory docstring listed a float into decimal among the lossy conversions that pass without a word, rounding to the declared scale. Both routes refuse it: the sequence converter takes an int or a Decimal, and the typed one refuses a float dtype. The docstring says so, and a test holds it to that on both routes and passes a Decimal through.
The gate reads its interpreter from the floor of requires-python, and the invariants workflow runs it against this tree's own ">=3.12" alone, so the ~= clause it also accepts and the refusal of a set naming neither had no run behind them. The test loads the script by its path and skips where the tree carries no .github, as the catalogue's copies do not.
The gate imports tomllib, which arrived in Python 3.11, and the matrix runs 3.10 cells below the declared floor on purpose, so the test errored there with ModuleNotFoundError and fail-fast cancelled the rest of the matrix. The test now skips where the script cannot load, as it does where the tree carries no .github.
… one reached numba as locals
…ACHE_DIR has no effect there
…tten; the import died on its OSError
…26-09-25 Close what a whole-tree pass found after #11: the output side's row shapes and time units, two aborts, a use-after-free, and a cache that could not be written
MiniMax Code ReviewThis is a large pull request with many changes. Let me analyze the key modifications:
The modifications aim to strengthen type safety, improve error reporting, and ensure consistent handling of edge cases across various data types and Arrow implementations. Comprehensive test coverage added to validate:
The modifications address nuanced edge cases in data type conversions, field validation, and type handling across multiple data processing scenarios. Code ReviewThis is a substantial PR with changes spanning multiple areas: datetime/timedelta handling, caching improvements, error messaging enhancements, and test coverage. Let me provide feedback on the key changes. Summary of Changes1. Datetime/Timedelta Unit Handling (
|
Owner
Author
|
From the fake Slim Shady: On the review of the sync merge. The content is what upstream merged as Goykhman#12, so only the checkable points. The match on "no locator available": numba raises a plain Nothing to change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merges
upstream/mainat 9fffb0b, the merge of Goykhman#12, into fork main. Git merged it clean, and the tree is identical to c18dc7a, the head of #34, which ran green on the full fork matrix.