Repository navigation
Make map() keep its column types, ENUMs included - #744
evertlammerts wants to merge 11 commits into
Conversation
…he constant NULL column, the declared bind stands
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes. The premise holds: on the released 2.0.0 an identity map() over an ENUM column returns TINYINT codes with -1 for NULL, a STRUCT(x BIGINT) column fails with "UDF column type mismatch", and project("col0 as a, col1 as a") loses a column. On this head all three round trip. The approach (fix one type per column at bind, read each chunk into it) fits how con.append already reads declared columns, and the ENUM and duplicate name parts look right to me. The blocker is the rule for which types get imposed. It now casts values that the function deliberately changed into another kind back into the input's type, silently, in cases where the base raised an error. The description argues against exactly that for typed inputs, and UNION is the worst case, because the tag is lost.
Findings, most severe first
-
Silent casts where the base raised (src/map.cpp:113-114 and 268-273). Two implicit sources now impose their type. One is the object input of the same name; the other is the type the empty bind frame gave the column. Either way the cells are cast into that type whatever they hold. Probed on this head, each of these raised "UDF column type mismatch" on 2.0.0:
s.apply(lambda d: {"x": str(d["x"]).zfill(5)})overSTRUCT(x BIGINT)reads{'x': 1}.- The same zfill over
INTEGER[]reads[1, 7]. s.apply(json.loads)over VARCHAR reads the text{'a': 1}.a.apply(lambda x: Decimal(x) / 3)over BIGINT reads 0, 1, 1, becauseapplyon an empty int64 Series keeps int64.
Ask: keep the cast for
schema=, where the user stated the type. For the two implicit sources, refuse a cell whose own type the column cannot take without changing its meaning, the way the guessed path already refuses. One option is to accept only casts the engine would apply implicitly. Add these four cases as tests. -
UNION loses its tag (src/map.cpp:114, tests/fast/test_map_types.py:92). A UNION reaches the function as its member's value only, and the imposed cast then picks a member again.
union_value(b := NULL::VARCHAR)comes back as a NULL UNION.- In
UNION(a INTEGER, b BIGINT),b := 7comes back taggeda. - On pandas 3, a chunk whose members are all strings still fails with a type mismatch, because pandas turns that object array into its str dtype.
Ask: do not impose a UNION from an object input, which leaves it refused as before, unless the tag can travel. The UNION test should check
union_tag(u)with both members, a NULL member and overlapping member types;fetchall()cannot tell a NULL member from a NULL UNION. -
A guess is confirmed only by an exact type match (src/map.cpp:283-289). The analyzer types Python ints in the int32 range as INTEGER. So
select i as a from range(3)mapped withdf.assign(a=df["a"].astype(object))is refused, while the same function over values above 2**31 works. BIGINT is the most common integer type, and the base fails this way too. But the guess rule is new here, and its test uses the one input type, INTEGER, where it works. Ask: confirm a guess when the chunk's type widens losslessly into it (INTEGER into BIGINT, for example), and test the guess with a BIGINT input. -
Benchmarks for object columns that now go through the declared conversion. Before this change, identity maps of BLOB, UUID, TIME,
INTEGER[]andSTRUCT(x INTEGER)worked through the analyzer and the object scan. Now every cell takesTransformPythonValueand the engine's cast, and the description measures only VARCHAR and ENUM. Ask: base against head, Release builds, one thread, 1M rows, on pandas 2 and 3, for:- identity maps of those five types;
- an identity map of a BIGINT column, to show what the new per chunk Python calls cost;
- a map that adds a column of only None, which hits the new
notna().any()check.
Nits, optional
- The refusal of a guessed column at src/map.cpp:288 says "has no type: the function returned no values for it at bind time", while
relation.typesshows the guessed type. The base said "expected [BIGINT], got [DOUBLE]", which is more useful; naming the guess and the chunk's type would be better. - A new column of
duckdb.NullValue()cells read as a NULL column on 2.0.0 and is now refused. The description says so and a test pins it.NullValueis the client's own NULL marker, though, so reading it as missing in that check would be more coherent. - The VARCHAR fast path at src/numpy/numpy_scan.cpp:481 builds a
std::stringand then copies it again withAddString, andWritestill runs when every row was written directly. declared_types(src/map.cpp:268-273) never changes after bind and could live inMapFunctionData. With finding 3 fixed, the SQLNULL acceptance inMatchesBoundTypes(src/map.cpp:226) looks unreachable, and its comment still refers to the old rule.- Functions that did arithmetic on ENUM codes now get a Categorical. That matches
df()and is right, but it deserves a line in the release notes for the 2.0 line. - Identity maps of JSON columns still fail on pandas 3 (str dtype, then VARCHAR against JSON). That was already so before this change, so it is a follow-up.
Outside findings I did not take
- The deleted
NULL_typed_Value_for_BIGINTcase: aduckdb.Valueof type NULL is input nobody writes, and the description explains the change. - Integer columns with a NULL reaching the function as float64: this predates the change, and the description scopes it out.
- Case-insensitive deduplication of input names: it matches
df(), and SQL inputs already arrive asa,A_1on 2.0.0. - Moving the ENUM identity lookup into a cache inside the shared categorical path: it would add shared state for no gain over a parameter only
map()passes. - The column name prefix appearing only on declared columns' errors: message polish.
- Missing ENUM tests at code width boundaries: I probed 127, 128, 255, 256 and 40000 values with NULLs, identity and
df.copy(), and all round trip.
What I verified (head 891364f, range c0f203b..891364f, Release build, Python 3.13, pandas 3.1.1, against the duckdb 2.0.0 wheel)
- The probes behind every finding above, run on this build and on 2.0.0.
pytest -n 8over test_map_types, test_map, test_map_lifetime, the appender tests, test_append_df, the pandas enum and category tests, tests/fast/numpy and tests/fast/udf: 1909 passed, 2 expected failures.- CI: every completed check passes; five abi3 test jobs were still running when I looked.
| vector<LogicalType> declared_types; | ||
| for (idx_t col_idx = 0; col_idx < output.ColumnCount(); col_idx++) { | ||
| auto &type = data.out_types[col_idx]; | ||
| auto imposed = type.id() != LogicalTypeId::SQLNULL && !data.guessed[col_idx]; |
There was a problem hiding this comment.
auto imposed = type.id() != LogicalTypeId::SQLNULL && !data.guessed[col_idx];
This imposes the type for every column bind typed, not only for schema= or an object input as the comment above says, and line 114 imposes the type of an object input of the same name. Either way, cells the function changed in kind are cast back silently, where the base raised a type mismatch. On this head: a.apply(lambda x: Decimal(x) / 3) over BIGINT reads 0, 1, 1 (empty int64 apply keeps int64 at bind); s.apply(lambda d: {"x": str(d["x"]).zfill(5)}) over STRUCT(x BIGINT) reads {'x': 1}; the same over INTEGER[] reads [1, 7]; s.apply(json.loads) over VARCHAR reads the text {'a': 1}. Please keep the cast for schema= and refuse, for the implicit sources, a cell whose own type the column cannot take without changing its meaning (for example, accept only implicit casts), with these cases as tests.
| return_type = original_type; | ||
| auto &input_type = input_types[input.GetIndex()]; | ||
| return_types[col_idx] = input_type; | ||
| guessed[col_idx] = RawArrayWrapper::DuckDBToNumpyDtype(input_type) != "object"; |
There was a problem hiding this comment.
guessed[col_idx] = RawArrayWrapper::DuckDBToNumpyDtype(input_type) != "object";
UNION counts as an object input, so it is imposed. But the function gets only the member's value, and the cast picks a member again: union_value(b := NULL::VARCHAR) comes back as a NULL UNION, and in UNION(a INTEGER, b BIGINT) the value b := 7 comes back tagged a. On 2.0.0 both raised. Please do not impose a UNION from an object input unless the tag can travel, and extend the UNION case at tests/fast/test_map_types.py:92 to check union_tag(u) with both members, a NULL member and overlapping member types.
| // it, and the analyzer's sample decides which, as it did before for every column. | ||
| vector<bool> missing_values(output.ColumnCount(), false); | ||
| for (idx_t col_idx = 0; col_idx < output.ColumnCount(); col_idx++) { | ||
| if (!data.guessed[col_idx] || pandas_return_types[col_idx] == data.out_types[col_idx]) { |
There was a problem hiding this comment.
if (!data.guessed[col_idx] || pandas_return_types[col_idx] == data.out_types[col_idx]) {
Only an exact match confirms a guess, and the analyzer types Python ints in the int32 range as INTEGER. So select i as a from range(3) mapped with df.assign(a=df["a"].astype(object)) is refused, while values above 2**31 pass: the result depends on the data, for the most common integer type. Please confirm a guess when the chunk's type widens losslessly into it, and add a BIGINT case beside the INTEGER one in test_map_typed_input_column_the_function_turned_into_objects_keeps_a_confirmed_type.
… UNION a guess, confirm a guess by lossless widening, and cast MAP keys as values
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes. The three behaviour asks from round 1 are answered on this head, and I checked each against my own round 1 probes: a function that changes what a column holds is now refused instead of cast back (Decimal(x) / 3 over BIGINT, zero padded text in a STRUCT(x BIGINT) or an INTEGER[], json.loads over VARCHAR, on pandas 2 and 3); the identity of a UNION is refused again instead of losing its tag; and Python ints for a BIGINT input are taken whether or not they fit 32 bits. The premise still holds (ENUM codes, STRUCT(x BIGINT) and duplicate names on 2.0.0, all fixed here). The benchmark ask is answered, and my own rerun agrees for every case named. The new commit brings five problems, though. In a column whose type holds a MAP, a cell that is not a dict now raises an internal error that invalidates the database, and that path also serves con.append. The commit does not compile under the stable ABI, so the abi3 wheels cannot build. A column whose input SQL typed as NULL now silently drops what the function writes into it. Lists of SMALLINT or TINYINT got 35 to 50 percent slower. And the kind check accepts some values that were rounded before it ran. The rewritten description also needs work before it is pasted: it carries internal identifiers and a few claims the code does not bear out.
Findings, most severe first
-
In a column whose type holds a MAP, a cell that is not a dict invalidates the database (src/numpy/numpy_scan.cpp:351, src/native/python_conversion.cpp:246).
DeclaredValuenow converts every cell of such a column with the MAP's shape as its target, and a shape holds UNKNOWN. A str, an int or a float cell then reaches the cast intoMAP(UNKNOWN, UNKNOWN)and fails with "INTERNAL Error: Invalid PhysicalType for GetTypeIdSize". After that, every query on the connection fails with "database has been invalidated". I probed this, each case on a fresh connection:con.appendof a frame whose object column holds the text"{a=1}"into aMAP(VARCHAR, INTEGER)column reads{a=1}on the base. Here it kills the database.- A
map()that returns 0 in a MAP column raised "UDF column type mismatch" on the base. Here it kills the database too.
The same shape also unifies a dict's values before the cast:
{"a": 2**60 + 1, "b": 1.0}appended intoMAP(VARCHAR, BIGINT)stores1152921504606846976here and the exact value on the base, because the values are made DOUBLE first. Ask: use the shape only for a cell that is a dict or a list, and have each key and value converted toward the key and value types of the target rather than toward their common type. Test the text cell, a scalar cell under map(), and the mixed int and float dict. -
The stable ABI build breaks (src/numpy/numpy_scan.cpp:894-895). The new BLOB fast path uses
PyBytes_AS_STRING,PyByteArray_AS_STRING,PyBytes_GET_SIZEandPyByteArray_GET_SIZE, which onlycpython/bytesobject.handcpython/bytearrayobject.hdeclare, and those headers are skipped underPy_LIMITED_API. I compiled the file with the tree's own compile command plus-DPy_LIMITED_API=0x030C0000: four "was not declared in this scope" errors on this head, none on the base. The editable build keeps the full API, so local tests cannot show it. Ask: use the limited API functions (PyBytes_AsStringAndSize,PyByteArray_AsStringwithPyByteArray_Size). -
A column SQL typed as NULL silently drops the function's values (src/map.cpp:131-135).
con.sql("select i a, NULL c from range(3) t(i)").map(lambda df: df.assign(c=pd.Series(["x"] * len(df), dtype=object, index=df.index)))returns(0, None), (1, None), (2, None)on this head; with ints incit does the same. On 2.0.0 and on the base both raise "UDF column type mismatch". A secondmap()that fills a column a firstmap()added as None loses its values the same way.OverrideNullTypecopies the input's NULL type,DuckDBToNumpyDtypeof NULL is "object", so the column counts as inferred, and the check that an untyped column holds no value (line 391) runs only for untyped ones. Line 397 then writes a constant NULL. Ask: an input column of type NULL should lend no type, so the column takes the "has no type ... pass its type with schema=" error; test both the SQL NULL column and the chained map. -
Narrow integer lists got 35 to 50 percent slower (src/numpy/numpy_scan.cpp:626 and 694-698). A Python int converts to INTEGER, so every element of a
SMALLINT[]orTINYINT[]takes the by-value path:Judgeagain per cell, then twoDefaultTryCastAsper element. One thread, 1M rows, pandas 3, the best of three in two alternating runs:SMALLINT[]identity 2.63 s on the base, 3.95 s here;TINYINT[]2.69 s against 3.89 s. Both identities worked on the base.INTEGER[]got faster (2.76 s against 2.00 s), which is why the case I named in round 1 did not show this. Ask: check that an integer fits a narrower integer type by its range, without building Values, and cache the verdict of a nested type per chunk rather than judging it again for each cell; addSMALLINT[]and aSTRUCTwith a SMALLINT field to the benchmark table. -
The kind check accepts values that were rounded before it ran (src/numpy/numpy_scan.cpp:803-805 and 609). First,
SignificantDigitscounts the digits of the float's shortest text, which cannot show a rounding thatdf()made before the function ran. The identity of[1.000000000000000001::DECIMAL(19,18)]returns1.000000000000000000, and that of[100000000000000.001::DECIMAL(18,3)]returns100000000000000.000. Second, the conversion drops the nanoseconds of apd.Timestampbefore the check sees it. Soselect '2020-01-01 00:00:00.000000001'::TIMESTAMP_NS as tmapped withdf.assign(t=df["t"].astype(object))returns2020-01-01 00:00:00. All three raised a mismatch on 2.0.0 and on the base, and the description says rounded identities are refused. Ask: make the float rule sound, for example count a DECIMAL's digits through its scale, or refuse floats for a DECIMAL wider than 15 digits. Keep the nanoseconds of apd.Timestampor adatetime64[ns]cell in the conversion. Add these three cases as tests. -
The rewritten description is not ready to paste. It names four decisions by internal question identifiers, cites "#8", a pull request of the author's fork, where duckdb/duckdb-python knows it as #746, and keeps a process note ("this sentence goes once it is answered"). Some claims do not hold: the rounding claim in finding 5. The cost of the new check for a column of only None is given as 0.07 G instructions, about 4 percent on pandas 2, but wall and CPU time grow 16 to 18 percent on both pandas versions (0.121 s against 0.141 s on pandas 2, 0.185 s against 0.217 s on pandas 3), which the author's own wall ranges show too. "Three keys now read differently" in
con.appendleaves out bool keys (Trueread "True", now "true"), timedelta keys ("1 day, 0:00:00", now "1 day") and tuple keys ("(1, 2)", now "[1, 2]") for aMAP(VARCHAR, ...). And 17 map lifetime tests fail on pandas 2, not 13, the same 17 on the base. Ask: replace those passages, and state the open behaviours in plain words.
Four behaviours the author marks as open with the maintainer. I judged what is built for each, as an interim state. All four are acceptable to merge as built, provided the description lists them as behaviour changes:
- A text column that mixes str with other scalars without
schema=is refused. On pandas 2,df["s"].fillna(0)read'0'on the base and now raises "expected [VARCHAR], got [INTEGER]". It fails loudly andschema=restores the old reading. - An object without a DuckDB type (a set, a Fraction, an IP address) in an inferred text column is refused by the conversion's error. It also fails loudly.
- Nested TIMESTAMP_NS and TIME_NS identities round trip at microseconds. The function only ever saw microseconds, so this reads back what it returned.
- A container whose elements the conversion unifies is read as unified:
{"a": 1, "b": "x"}intoMAP(VARCHAR, VARCHAR)reads'1', while a top level int in a VARCHAR column is refused. That is inconsistent, but no value is lost, andcon.appendreads such a container the same way.
Nits, optional
- A chunk holding only NULL members of a UNION reads as NULL UNIONs. The base does the same (probed), so this predates the change.
- The TIME identity is 6 to 7 percent slower (0.454 s against 0.485 s on pandas 3, 0.400 s against 0.426 s on pandas 2). A fast path for an exact
datetime.time, like the ones for str and bytes, would remove that. - The comment on
MapFunctionData::declared_types(src/map.cpp:48) still says "INVALID where the analyzer types it", butDeclareTypesnever writes INVALID. - An untyped column whose chunk comes back typed (an int64 Series) now gets the plain mismatch message, not the "has no type ... pass its type with schema=" message the base gave first.
IntegerRangeandWidensLosslessly(src/map.cpp:296-333) restate integer widening thatImpliedKinds::Judgealready gets fromCastFunctionSet::ImplicitCastCost. The BLOB into BIT rule inJudge(numpy_scan.cpp:620) duplicatesbit_storage_refused, which map() always sets.- A test name and three test comments say "the base" (test_map_types.py:579 and 701, test_append_df.py:486 and 493). A reader of the file does not know which base that is; "2.0.0" would say it.
Benchmarks I reran (one thread, 1M rows, Release builds of the base c0f203b and this head with identical settings, best of five, two runs alternating, base against head)
| case | pandas 3 | pandas 2 |
|---|---|---|
| VARCHAR identity | 0.247 / 0.254 s | 0.162 / 0.164 s |
| UUID identity | 2.188 / 2.142 s | 1.925 / 1.960 s |
| BLOB identity | 0.397 / 0.219 s | 0.313 / 0.155 s |
| TIME identity | 0.454 / 0.485 s | 0.400 / 0.426 s |
| INTEGER[] identity | 2.761 / 1.998 s | 2.683 / 1.932 s |
| STRUCT(x INTEGER) identity | 2.507 / 1.792 s | 2.378 / 1.739 s |
| BIGINT identity | 0.100 / 0.095 s | 0.057 / 0.056 s |
| add a column of None | 0.185 / 0.217 s | 0.121 / 0.141 s |
| SMALLINT[] identity | 2.63 / 3.95 s | not run |
| TINYINT[] identity | 2.69 / 3.89 s | not run |
These agree with the author's table in direction and size for every row except the None column, where the author's instruction count understates the time.
Outside findings I did not take
- A list or MAP whose elements the conversion unifies before the kind check: this is the fourth open behaviour above, built as the author documents it.
- TIMESTAMPTZ and TIMESTAMPTZ_NS judged to fit each other without a value check: no Python value converts to TIMESTAMPTZ_NS (an aware nanosecond
pd.Timestampconverts to TIMESTAMPTZ), so the path is not reached. - A UNION chunk of only NULL members reads as a NULL UNION: the base does the same; it is listed as a nit.
HoldsValueand zero dimensional numpy arrays: input nobody writes into a new column.- Explicit
schema=with a NULL type: input nobody writes; the NULL typed input column in finding 3 is the ordinary trigger. - STRUCT or MAP holding an ENUM, and VARIANT, identities refused: the base refuses them with the same message (probed).
- An empty list for an ARRAY of MAP fails an assertion in debug builds: a Release build raises the cast's length error, as the base does (probed), and the input is rare.
- A schema stated as NULL drops values, and a
duckdb.Valuestated as BLOB is refused for a BIT schema: input nobody writes. - A zoned value of the year 1500 into an implied TIMESTAMPTZ_NS reads the cast's error instead of the mismatch message: it fails loudly, so this is message polish.
What I verified (head b504c5e, range c0f203b..b504c5e without the upstream side of the merge, Release builds of head and base, Python 3.13, pandas 3.1.1 and 2.3.3, against the duckdb 2.0.0 wheel)
- My round 1 probes, rerun on this head, and new probes for every finding above, on the head, the base and 2.0.0.
- The code-review skill (xhigh, the whole range) and two Codex reviews; every finding I took I reproduced.
- The limited API compile of src/numpy/numpy_scan.cpp on the head and the base.
pytest -n 8on pandas 3 over test_map_types, test_map_lifetime, test_map, the appender tests, test_append_df, the pandas enum and category tests, tests/fast/numpy and tests/fast/udf: 2031 passed, 2 expected failures.- On pandas 2, test_map_types, test_map and test_append_df pass. 17 test_map_lifetime tests fail, and the same 17 fail on the base.
- The head's test_map_types run against the base build: 95 of 177 fail, as the description says.
| return Value(TransformPythonString(nb::str(cell))); | ||
| } | ||
| return TransformPythonValue(context, cell, LogicalType::UNKNOWN, true); | ||
| return TransformPythonValue(context, cell, shape ? *shape : LogicalType(LogicalTypeId::UNKNOWN), true); |
There was a problem hiding this comment.
return TransformPythonValue(context, cell, shape ? *shape : LogicalType(LogicalTypeId::UNKNOWN), true);
For a column whose type holds a MAP every cell converts with the shape, which holds UNKNOWN, so a str, int or float cell reaches a cast into MAP(UNKNOWN, UNKNOWN) and fails with "INTERNAL Error: Invalid PhysicalType for GetTypeIdSize"; the database is invalidated afterwards. Probed on fresh connections: con.append of the text "{a=1}" into MAP(VARCHAR, INTEGER) (reads {a=1} on the base), and a map() that returns 0 in a MAP column (a type mismatch on the base). Please use the shape only for a dict or list cell, and test both.
| @@ -241,10 +251,11 @@ Value TransformStructFormatDictionaryToMap(optional_ptr<ClientContext> context, | |||
|
|
|||
There was a problem hiding this comment.
Under the MAP shape the keys and values convert with UNKNOWN and the MAP takes their common type (lines 244-245), so {"a": 2**60 + 1, "b": 1.0} appended into MAP(VARCHAR, BIGINT) becomes DOUBLE first and stores 1152921504606846976; the base stored the exact value. Please convert each key and value toward the target's key and value types instead of toward their common type, with this dict as a test.
| // df() hands a BLOB cell to the function as a bytearray | ||
| if (plain_blob && (PyBytes_CheckExact(cell.ptr()) || PyByteArray_CheckExact(cell.ptr()))) { | ||
| auto bytes = PyBytes_CheckExact(cell.ptr()); | ||
| auto data = bytes ? PyBytes_AS_STRING(cell.ptr()) : PyByteArray_AS_STRING(cell.ptr()); |
There was a problem hiding this comment.
auto data = bytes ? PyBytes_AS_STRING(cell.ptr()) : PyByteArray_AS_STRING(cell.ptr());
These four macros (with PyBytes_GET_SIZE and PyByteArray_GET_SIZE on the next line) are declared only in cpython/bytesobject.h and cpython/bytearrayobject.h, which are skipped under Py_LIMITED_API. Compiling this file with the tree's compile command plus -DPy_LIMITED_API=0x030C0000 gives four "was not declared in this scope" errors here and none on the base, so the abi3 wheels cannot build. The editable build keeps the full API, so local tests pass. Please use PyBytes_AsStringAndSize, and PyByteArray_AsString with PyByteArray_Size.
| } | ||
| auto &input_type = input_types[input.GetIndex()]; | ||
| return_types[col_idx] = input_type; | ||
| auto object_input = RawArrayWrapper::DuckDBToNumpyDtype(input_type) == "object"; |
There was a problem hiding this comment.
auto object_input = RawArrayWrapper::DuckDBToNumpyDtype(input_type) == "object";
For an input column of type NULL this is true, so the column counts as inferred with type NULL, the untyped check at line 391 skips it, and line 397 writes a constant NULL. con.sql("select i a, NULL c from range(3) t(i)").map(lambda df: df.assign(c=pd.Series(["x"] * len(df), dtype=object, index=df.index))) returns (0, None), (1, None), (2, None), and so does a second map() that fills a column a first map() added as None. On 2.0.0 and on the base both raise "UDF column type mismatch". Please let an input of type NULL lend no type, so the column gets the "has no type ... pass its type with schema=" error, and test the SQL NULL column and the chained map.
| return false; | ||
| } | ||
| // The type holds the value when the cast into it and back gives the value again | ||
| auto cast = value.DefaultTryCastAs(type); |
There was a problem hiding this comment.
auto cast = value.DefaultTryCastAs(type);
A Python int converts to INTEGER, so each element of a SMALLINT[] or TINYINT[] comes here (after Judge runs again for the cell at line 648) and takes two Value casts. One thread, 1M rows, pandas 3, best of three over two alternating runs: SMALLINT[] identity 2.63 s on the base against 3.95 s here, TINYINT[] 2.69 s against 3.89 s. Both worked on the base. Please check an integer against the target's range without building Values, cache the verdict of a nested type per chunk, and add these cases to the benchmark table.
| return false; | ||
| } | ||
| auto text = Value::DOUBLE(number).ToString(); | ||
| if (SignificantDigits(text) > MAX_EXACT_FLOAT_DIGITS) { |
There was a problem hiding this comment.
if (SignificantDigits(text) > MAX_EXACT_FLOAT_DIGITS) {
The float's shortest text cannot show a rounding df() made before the function ran. The identity of [1.000000000000000001::DECIMAL(19,18)] returns 1.000000000000000000, and that of [100000000000000.001::DECIMAL(18,3)] returns 100000000000000.000; both raised a mismatch on 2.0.0 and on the base, and the description says such identities are refused. Please make the rule sound, for example by counting a DECIMAL's digits through its scale, or by refusing floats for a DECIMAL wider than 15 digits, and add these two cases as tests.
The same holds for nanoseconds: the conversion truncates a pd.Timestamp to microseconds before the check (line 609 then accepts it), so select '2020-01-01 00:00:00.000000001'::TIMESTAMP_NS as t mapped with df.assign(t=df["t"].astype(object)) returns 2020-01-01 00:00:00, where the base refused it.
…ainer cells toward their key and value types, keep nanoseconds and check narrow integers by range
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes. The six asks from round 2 are answered on this head, and I checked each with my round 2 probes: a str or an int cell in a MAP column now meets a normal error and the database stays usable; {"a": 2**60 + 1, "b": 1.0} appended into MAP(VARCHAR, BIGINT) keeps the exact value; every touched C++ file compiles under the stable ABI (Python's limited API) and the Windows abi3 seed wheels that failed in round 2 now pass in CI; a column filled from an input SQL typed as NULL raises the "has no type ... pass its type with schema=" error; SMALLINT[] and TINYINT[] identities are now 20 percent faster than the base instead of 50 percent slower; and the rounded DECIMAL list identities and the TIMESTAMP_NS column handed back through astype(object) are refused or round trip exactly. The premise still holds (ENUM codes, STRUCT(x BIGINT), duplicate names on 2.0.0, all fixed here). The new commit brings two regressions and a slowdown, though. The {"key": [...], "value": [...]} form of a MAP does not follow the new shape rules: in map() it can invalidate the database, and in con.append it now refuses values that 2.0.0 and the base read. A pandas Timestamp outside the years 1677 to 2262 (a 9999-12-31 end date, for one) now fails with a raw pandas OverflowError in an inferred TIMESTAMP column. And the identity of a TIMESTAMP column handed back as pandas Timestamps is 20 to 28 percent slower. The rewritten description is otherwise accurate and lists the four open behaviours as changes from 2.0.0, but it must be updated for these three before it is pasted.
Findings, most severe first
- The key and value list form of a MAP bypasses the shape rules (src/native/python_conversion.cpp:411-416, reached from src/numpy/numpy_scan.cpp:361).
TransformDictionaryToMapconverts thekeyandvalueentries with a plainLIST(...)target built from the shape, so neitherShapeTakesnorTransformMapEntryruns for them. For an inferred MAP column the target isLIST(UNKNOWN): a map() returning{"key": "ab", "value": "cd"}(a two entry MAP whose keys happen to be "key" and "value") for aMAP(VARCHAR, VARCHAR)column raises "INTERNAL Error: UDF column 'a': Invalid PhysicalType for GetTypeIdSize" and every later query fails with "database has been invalidated"; the base and 2.0.0 refuse it with a normal cast error. For a stated type, line 361 now keeps the plain entry type, so the list converts straight into it, andcon.appendof{"key": ["k"], "value": [datetime(2024, 5, 6)]}intoMAP(VARCHAR, TIMESTAMPTZ)fails with "Could not convert 'datetime' to type TIMESTAMP WITH TIME ZONE"; bytes into aVARCHARvalue,1.5intoDECIMAL(9,2)and a datetime intoTIMESTAMP_NSfail the same way. 2.0.0 and the base read all four, and the dict form of the same values reads on this head. Ask: give the key and value lists the same rules as a dict's entries (shape for containers,TransformMapEntryfor plain types), and test the scalar{"key": "ab", "value": "cd"}case in map() and these fourcon.appendcases. - A pandas Timestamp outside the nanosecond range fails in an inferred timestamp column (src/native/python_conversion.cpp:251).
HoldsNanosecondsreadsTimestamp.value, which raises OverflowError for any Timestamp outside 1677 to 2262, andDeclaredValuecalls it for every cell of an inferred TIMESTAMP, TIMESTAMP_MS or TIMESTAMP_S column and every nested timestamp shape.con.sql("select '9999-12-31'::TIMESTAMP t").map(lambda df: df.assign(t=df.t.astype(object)))fails with a pandas traceback, "Cannot convert Timestamp to nanoseconds without overflow", on pandas 2 and 3; so do the years 1500 and 2500, a mixed column of 2020 and 9999, and{"t": pd.Timestamp("9999-12-31")}for aSTRUCT(t TIMESTAMP). The base and 2.0.0 read them all. Ask: take the nanosecond path only when the Timestamp has nanoseconds (nanosecond != 0, which also means it is in range), and test these dates at the top level and as a field. - TIMESTAMP identity through pandas Timestamps is 20 to 28 percent slower (src/numpy/numpy_scan.cpp:336). Every pd.Timestamp cell of an inferred naive timestamp column now becomes a TIMESTAMP_NS Value through several Python attribute reads, and the kind check then casts it by value back into TIMESTAMP. One thread, 1M rows, pandas 3, best of three in two alternating runs:
df.assign(c=df.c.astype(object))over a TIMESTAMP column takes 1.60 and 1.70 s on the base against 2.04 and 1.98 s here. Ask: convert such a cell as before when it has no nanoseconds (the fix for finding 2 may cover this), and add this case to the benchmark table. - The description needs these three changes reflected before it is pasted. It says that a shaped container takes only the object it is for "at every depth", which finding 1 contradicts, and its benchmark table has no row for a TIMESTAMP column handed back as Timestamps. It still carries two process references ("since review round 2", "in the reviewer's runs"), which a reader of the merged history cannot place. The rest I checked against the code and my probes holds, including 126 of 257 new tests failing on the base and the four open behaviours, stated in their own paragraph as changes from 2.0.0. Ask: update those passages after the fixes.
Nits, optional
ShapeTakes(src/native/python_conversion.cpp:299) accepts a numpy array of no dimensions for a LIST shape;[np.array(5)]returned for aMAP(VARCHAR, INTEGER)[][]column invalidates the database, and withschema=the message names the internal aliasduckdb_py_shape. Nobody writes that input, butndim > 0would close it.- Marking a shape with a type alias puts a conversion mode inside
LogicalType, where it takes part in equality and printing; a parameter of the conversion would say the same without the alias reaching a Value or a message. - A float is refused for any DECIMAL of scale above 15, also for
0and0.0001in aDECIMAL(18,16)list. The rule is sound and the base refuses these too, so it fails loudly; counting digits from the value's magnitude would accept them. - An input typed
"NULL"[]orSTRUCT(x "NULL")still lends that type, so filling such a column gives the plain mismatch rather than the new "has no type" hint.
Outside findings I did not take
- An aware nanosecond Timestamp loses its nanoseconds into TIMESTAMPTZ: the base and 2.0.0 do the same (probed).
TIMESTAMP_NS[], TUPLE and nested ENUM identities are refused: the base and 2.0.0 refuse them too (probed).[2**60 + 1, 1.0]appended intoBIGINT[]rounds: the base and 2.0.0 do the same; round 2 asked for the MAP case only.{"k": np.int64(5)}appended intoMAP(VARCHAR, BIGINT)raises NotImplemented, and bytes into aMAP(VARCHAR, BIT)value read as bits: the base does the same (probed).- A schema stated as NULL drops values: input nobody writes, as in round 2.
- The
UNIONtests besideIsNested()are redundant: no effect.
Benchmarks I reran (one thread, 1M rows, Release builds of the base c0f203b and this head, best of three or five, two runs alternating, base against head, at a machine load near 100 on 192 cores)
| case | pandas 3 | pandas 2 |
|---|---|---|
| SMALLINT[] identity | 2.59, 2.54 / 2.09, 1.99 s | not run |
| TINYINT[] identity | 2.50, 2.48 / 1.91, 1.91 s | not run |
| STRUCT(x SMALLINT) identity | refused / 1.87, 1.80 s | not run |
| VARCHAR identity | 0.237 / 0.267 s | 0.162 / 0.167 s |
| UUID identity | 1.91 / 2.01 s | 1.88 / 1.83 s |
| BLOB identity | 0.379 / 0.235 s | 0.308 / 0.139 s |
| TIME identity | 0.508 / 0.320 s | 0.390 / 0.256 s |
| INTEGER[] identity | 2.47 / 1.90 s | 2.36 / 1.75 s |
| STRUCT(x INTEGER) identity | 1.95 / 1.71 s | 1.90 / 1.64 s |
| BIGINT identity | 0.099 / 0.103 s | 0.055 / 0.055 s |
| add a column of None | 0.195 / 0.217 s | 0.119 / 0.134 s |
| TIMESTAMP as pd.Timestamp objects | 1.60, 1.70 / 2.04, 1.98 s | not run |
Single figures are the better of the two runs. These agree with the author's table in direction for every row it has; VARCHAR and UUID moved between runs by more than the difference.
What I verified (head 2ed45dd, range c0f203b..2ed45dd without the upstream side of the merge, Release builds of head and base, Python 3.13, pandas 3.1.1 and 2.3.3, against the duckdb 2.0.0 wheel)
- My round 2 probes rerun on this head, and new probes for every finding above and every outside finding, on head, base and 2.0.0, each case on a fresh connection followed by a query.
- The limited API compile (the tree's compile command plus
-DPy_LIMITED_API=0x030C0000 -fsyntax-only) of src/map.cpp, src/native/python_conversion.cpp, src/numpy/numpy_scan.cpp, src/numpy/numpy_result_conversion.cpp, src/pandas/bind.cpp and src/pyresult.cpp: no errors; the same command on round 2's numpy_scan.cpp gives its four errors. - The code-review skill (xhigh, the whole range) and two Codex reviews; every finding I took I reproduced.
pytest -n 8on pandas 3 over test_map_types, test_map_lifetime, test_map, the appender tests, test_append_df, the pandas enum and category tests, tests/fast/numpy and tests/fast/udf: 2116 passed, 2 expected failures. On pandas 2, the same 17 test_map_lifetime failures as on the base, the rest pass. The head's test_map_types on the base build: 126 failed, 131 passed, as the description says.- CI on this head: code quality, submodule sanity and five of the six cp312 abi3 seed wheels (both Windows, both manylinux, macOS arm64) pass; macOS x86_64, the two cp311 sanity builds, the sdist and the full API build were still running, and the full wheel matrix had not started.
| static LogicalType KeyedShape(const LogicalType &type, bool typed_entries) { | ||
| auto entry = [&](const LogicalType &entry_type) { | ||
| auto plain = !entry_type.IsNested() && entry_type.id() != LogicalTypeId::UNION; | ||
| return typed_entries && plain ? entry_type : KeyedShape(entry_type, typed_entries); |
There was a problem hiding this comment.
return typed_entries && plain ? entry_type : KeyedShape(entry_type, typed_entries);
For the {"key": [...], "value": [...]} form of a MAP, TransformDictionaryToMap (src/native/python_conversion.cpp:411-416) wraps this entry type in a plain LIST(...) and converts the lists straight into it, so neither ShapeTakes nor TransformMapEntry runs. Stated: con.append of {"key": ["k"], "value": [datetime(2024, 5, 6)]} into MAP(VARCHAR, TIMESTAMPTZ) fails with "Could not convert 'datetime' to type TIMESTAMP WITH TIME ZONE", and bytes into a VARCHAR value, 1.5 into DECIMAL(9,2) and a datetime into TIMESTAMP_NS fail too; 2.0.0 and the base read all four. Inferred: the target is LIST(UNKNOWN), so a map() returning {"key": "ab", "value": "cd"} for a MAP(VARCHAR, VARCHAR) column raises "INTERNAL Error ... Invalid PhysicalType for GetTypeIdSize" and invalidates the database, where the base refused it normally. Please convert the key and value lists by the same rules as a dict's entries and test both cases.
| !nb::getattr(cell, "tzinfo").is_none()) { | ||
| return false; | ||
| } | ||
| nanoseconds = timestamp_ns_t(nb::cast<int64_t>(cell.attr("value"))); |
There was a problem hiding this comment.
nanoseconds = timestamp_ns_t(nb::cast<int64_t>(cell.attr("value")));
Timestamp.value raises OverflowError outside 1677 to 2262, and this runs for every cell of an inferred TIMESTAMP, TIMESTAMP_MS or TIMESTAMP_S column. con.sql("select '9999-12-31'::TIMESTAMP t").map(lambda df: df.assign(t=df.t.astype(object))) fails with the pandas traceback "Cannot convert Timestamp to nanoseconds without overflow" on pandas 2 and 3, as do 1500, 2500 and {"t": pd.Timestamp("9999-12-31")} for a STRUCT(t TIMESTAMP); the base and 2.0.0 read them. Please take this path only when the Timestamp has nanoseconds (nanosecond != 0), and test these dates at the top level and as a field.
| (implied && (target.id() == LogicalTypeId::TIMESTAMP || target.id() == LogicalTypeId::TIMESTAMP_MS || | ||
| target.id() == LogicalTypeId::TIMESTAMP_SEC)); | ||
| timestamp_ns_t nanoseconds; | ||
| if (naive_timestamp && HoldsNanoseconds(cell, nanoseconds)) { |
There was a problem hiding this comment.
if (naive_timestamp && HoldsNanoseconds(cell, nanoseconds)) {
Every pd.Timestamp cell of an inferred TIMESTAMP column now becomes a TIMESTAMP_NS Value, which the kind check then casts by value. One thread, 1M rows, pandas 3, best of three in two alternating runs: df.assign(c=df.c.astype(object)) over a TIMESTAMP column takes 1.60 and 1.70 s on the base against 2.04 and 1.98 s here. Please convert a cell without nanoseconds as before and add the case to the benchmark table.
…mestamp as nanoseconds only when it has them, pick the declared cell reader once per vector
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes, for one performance regression in con.append that the new commit brings; everything else from round 3 is answered. I checked each round 3 ask with probes on this head. The {"key": [...], "value": [...]} form of a MAP now reads entry by entry: through con.append a naive datetime into MAP(VARCHAR, TIMESTAMPTZ), bytes into a VARCHAR value, 1.5 into DECIMAL(9,2) and a datetime into TIMESTAMP_NS read exactly as duckdb 2.0.0 reads them, and {"key": "ab", "value": "cd"} meets a normal "takes a list of keys and a list of values" error through map() (with and without schema=), con.append, the row appender and a query parameter, and the database stays usable (the merge base invalidated it). A 9999-12-31 pandas Timestamp now round trips in inferred TIMESTAMP, TIMESTAMP_MS and TIMESTAMP_S columns, at the top level, as a STRUCT field, a LIST element and a MAP value, on pandas 2.3 and 3.1. Timestamp.max and Timestamp.min, which df() hands out for an infinite TIMESTAMP_NS, read back as infinity. The TIMESTAMP identity through pandas Timestamps now takes 15 to 18 percent fewer instructions than the merge base, and con.append of naive object columns is at or below the base. The premise still holds (ENUM codes, STRUCT(x BIGINT) mismatch, duplicate names on 2.0.0, all fixed here). The pull request body on GitHub is still the old one that opens with "Merge after #8"; the rewritten description is accurate apart from the small points below, and it must replace the body before merge.
Findings, most severe first
con.appendof zone aware values into a TIMESTAMP column got slower (src/numpy/numpy_scan.cpp:1037-1038). The new TIMESTAMP fast path builds a wholePyDateTime(eight attribute reads) for every datetime cell and only then looks attzinfo; an aware cell throws that work away andread(cell, row)converts it again from scratch. Measured with Release builds, one thread, 1M cells, the calling thread's user space instructions (minimum of three, two alternating rounds) and wall time: awaredatetimeobjects into TIMESTAMP 8.00 G and 0.78 to 0.80 s on the base against 9.96 G and 0.87 to 0.88 s here (+24 percent instructions); aware pandas Timestamps on pandas 3.1 13.5 G and 1.28 to 1.30 s against 21.5 G and 1.95 to 2.03 s (+58 percent instructions, +52 percent wall); on pandas 2.3 8.09 G against 10.29 G (+27 percent). Aware datetimes into TIMESTAMPTZ and naive ones into TIMESTAMP are unchanged or faster. Ask: test the zone before building thePyDateTime(or hand the built one on to the conversion), so an aware cell costs what it cost on the base, and add the aware case to the benchmark table.
Nits the author may ignore
- The far year test (tests/fast/test_map_types.py:609) parametrizes
["TIMESTAMP", "TIMESTAMP_S"]but leaves outTIMESTAMP_MS, which round 3 found failing too; it passes on this head, so adding it is one word. - A numpy
datetime64[ns]array as the key or value component goes throughtolist()(src/native/python_conversion.cpp:459-462) and becomes integers, so{"key": ["a"], "value": np.array([...], dtype="datetime64[ns]")}intoMAP(VARCHAR, TIMESTAMP_NS)fails with "BIGINT -> TIMESTAMP_NS" while the dict form of the same value now reads its nanoseconds. The base and 2.0.0 fail the same way, so this is no regression; the description's sentence about such arrays could name this form too. - Whether a two entry dict with keys
keyandvalueis a MAP of its own entries or a refused key and value form depends on whether its two strings have equal length (DictionaryHasMapFormataccepts anything with a length). So the identity ofMAP {'key': 'ab', 'value': 'cd'}, whichdf()hands over as that plain dict, is refused, whileMAP {'key': 'ab', 'value': 'cde'}andMAP {'key': 1, 'value': 2}now round trip. The base and 2.0.0 refuse all three, so this is no regression, and I asked for the refusal in round 3; requiring list, tuple or array components in the format check (theShapeTakesAsListrule) would make all three round trip. Separately,{"value": [...], "key": [...]}is read by position and stores keys and values swapped, on every build including 2.0.0; that is a follow up, not part of this change. Timestamp.maxnow reads as infinity for an inferred TIMESTAMP column in map() and throughcon.appendinto TIMESTAMP_NS (where the base stored 2262-04-11 23:47:16.854775), but as the finite 2262 value throughcon.appendinto a stated TIMESTAMP and as a query parameter. That follows the rules (the value is TIMESTAMP_NS infinity, a stated coarser type reads microseconds), but the description's "which are howdf()hands DuckDB's infinities over" holds for TIMESTAMP_NS only (df()hands a TIMESTAMP, TIMESTAMP_MS or TIMESTAMP_S infinity over as a datetime64 of a far year, whose identity throughastype(object)fails on every build), and thecon.appendchange into TIMESTAMP_NS could be listed.- Whole second Timestamps appended into TIMESTAMP_NS take 3 to 5 percent more instructions (10.44 G against 10.73 to 11.01 G), since
NanosecondOfsends them through the datetime conversion; the comment at python_conversion.cpp:260-261 says that path costs less.
Outside findings I did not take
- Codex: the TIMESTAMP_MS test gap (kept as a nit above); the
datetime64array question (confirmed, kept as a nit, not a regression); errors cleared inComponentLengthandNanosecondOf(only reachable from objects whose__len__ornanosecondraise, adversarial). - code-review: positional read of
{"value", "key"}(confirmed on 2.0.0 and the base, outside this change);PyLong_AsLongLongoverflow left pending inNanosecondOf(needs ananosecondof 2**70, adversarial);KeyboardInterruptin a user__len__swallowed (adversarial); the replacement scan raising a raw TypeError for a 0 dimensional array component (unchanged from the base); repeated length and name probes per MAP cell, theSKIP_WRITTENtemplate, the fast paths besidePythonVectorConversion's own writers, the comment wording inTransformPythonValueand at python_conversion.cpp:151, and the separate interned name inNanosecondOf(style and small costs, no measured effect); the aware datetime cost (confirmed and measured, finding 1).
What I verified, on 6746e39 (range c0f203b..6746e39, the upstream side of the merge left out)
- Probes against Release builds of this head and of the merge base c0f203b, on pandas 3.1.1 and 2.3.3, and against the 2.0.0 wheel: the round 3 cases (key and value form through every caller, far years and infinity in every timestamp unit, top level and nested), the round 2 cases (a scalar in a MAP column, NULL typed input, rounded DECIMAL lists), and the new TIMESTAMP fast path against the general reader for 14 cell kinds (naive, Timestamp with and without nanoseconds, aware, NaT, NaN, None, a datetime subclass, text, numpy datetime64,
datetime.maxand.min, a date) plus a 5000 row column mixing fast and converted cells across chunks;con.appendreads every one exactly as the base does. - Benchmarks, instruction counts of the calling thread and wall time, Release, one thread, 1M rows, two rounds alternating base and head, pandas 3.1 and 2.3: every round 2 and round 3 case (VARCHAR, UUID, BLOB, TIME, INTEGER[], SMALLINT[], TINYINT[], FLOAT[], STRUCT(x INTEGER), STRUCT(t TIMESTAMP), STRUCT(d DATE), BIGINT, an added column of None, TIMESTAMP through
astype(object)) andcon.appendof eight object column kinds agree with the author's table to within a percent; nothing regressed again. The added column of None still costs 10 to 12 percent more instructions, as the description says. - The stable ABI: map.cpp, python_conversion.cpp, numpy_scan.cpp, numpy_result_conversion.cpp, pandas/bind.cpp and pyresult.cpp compile with the build's own command plus
-DPy_LIMITED_API=0x030C0000 -fsyntax-only, no errors. - Tests: test_map_types, test_map, test_map_lifetime, the appender tests, tests/fast/pandas, tests/fast/numpy and tests/fast/udf on pandas 3.1: 3481 passed, 132 xfailed; on pandas 2.3 the same 17 map lifetime tests fail as on the base and the rest pass; this head's test_map_types.py against the base build: 157 of 301 fail.
- CI at review time: code quality, submodule sanity, sdist, the full API build with assertions and the cp312 seed wheels for Windows (x86 64 and ARM64) and manylinux x86_64 pass; macOS and manylinux aarch64 seeds and the cp311 sanity jobs are pending; nothing is red.
| // A datetime, or a pandas Timestamp without nanoseconds, holds what the conversion reads | ||
| auto type = Py_TYPE(cell.ptr()); | ||
| if (type == datetime_type || (PyType_IsSubtype(type, datetime_type) && NanosecondOf(cell) == 0)) { | ||
| PyDateTime datetime(cell); |
There was a problem hiding this comment.
PyDateTime datetime(cell); reads all eight attributes of every datetime cell before if (datetime.tzone_obj.is_none()) decides; for a zone aware cell that work is discarded and read(cell, row) converts the cell again. con.append of 1M aware datetime objects into a TIMESTAMP column takes 9.96 G instructions here against 8.00 G on the base (+24 percent), and of aware pandas Timestamps on pandas 3.1 21.5 G against 13.5 G (1.95 to 2.03 s against 1.28 to 1.30 s). Ask: check the zone before building the PyDateTime, or pass the built one on, so an aware cell costs what it did on the base.
|
|
||
|
|
||
| @FAR_YEARS | ||
| @pytest.mark.parametrize("sql_type", ["TIMESTAMP", "TIMESTAMP_S"]) |
There was a problem hiding this comment.
Nit, optional: ["TIMESTAMP", "TIMESTAMP_S"] leaves out TIMESTAMP_MS, which failed the same way before this commit; it passes on this head, so adding it pins the third unit.
| // A shape reads the two lists entry by entry, as it reads a dict's own entries; a component that is no list | ||
| // converts without a target and is refused below | ||
| if (sized && ShapeTakesAsList(keys) && ShapeTakesAsList(values)) { | ||
| auto items = [](nb::handle list) { |
There was a problem hiding this comment.
Nit, optional: list.attr("tolist")() turns a datetime64[ns] array into integers, so {"key": ["a"], "value": np.array([...], dtype="datetime64[ns]")} into MAP(VARCHAR, TIMESTAMP_NS) fails with "BIGINT -> TIMESTAMP_NS" while the dict form of the same value keeps its nanoseconds. Same on the base and 2.0.0, so no regression; reading the array's elements as NumpyScalarItem reads a scalar would make the two forms agree.
… key and value form of a MAP by its components
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes, for a regression the new commit 66a35e7 brings and one wrong result in the same area. The round 4 finding is answered: con.append of zone aware values into a TIMESTAMP column now costs less than on the merge base, and I found no difference in what it reads. But the new rule for which dict is the key and value form of a MAP takes any dict with exactly the keys "key" and "value" as that form as soon as one side is a list, so a STRUCT with fields named key and value, one a list and one a plain value, no longer converts anywhere. The output of DuckDB's own map_entries is such a STRUCT, and 2.0.0, the merge base and the round 4 head all read it. The premise still holds (ENUM codes, STRUCT(x BIGINT) mismatch and duplicate names on 2.0.0, all fixed here). The pull request body on GitHub is still the old one that opens with "Merge after #8"; the rewritten description must replace it before merge, and its paragraph on the key and value form will need to follow the fix.
Findings, most severe first
-
A dict with keys "key" and "value", a list on one side and a plain value on the other, is now the key and value form everywhere (src/native/python_conversion.cpp:139 and 142).
if (key_none || value_none || !key_list || !value_list) { return true; }sends it to the MAP reading, which then refuses it, including where no MAP is asked for. The identity ofselect map_entries(MAP {'color': ['red', 'blue']}) e(aSTRUCT(key VARCHAR, value VARCHAR[])[]), with and withoutschema=,con.appendof itsdf()into the same type, andduckdb.sql("select * from df")over that frame all fail with "A dict of 'key' and 'value' for a MAP takes a list of keys and a list of values, not VARCHAR and VARCHAR[]" or a cast error; so doSTRUCT(key INTEGER[], value INTEGER)andSTRUCT(key INTEGER, value INTEGER[])through map(),con.append, the row appender, an inserted parameter andselect ?. A DataFrame scan of{'key': [1], 'value': 5}raises a raw PythonTypeError: object of type 'int' has no len()from the analyzer'sDictToMap, and{"key": [], "value": "ab"}reads as an empty MAP{}with no error throughselect ?and a DataFrame scan. 2.0.0 and the merge base read every one of these as its STRUCT (probed), and so does the rule of 6746e39 by its code, which needed both sides to have a length and the lengths to match. Ask: take the key and value form only when both sides are lists, tuples or arrays (of equal length), a list beside None, or None on both; a list beside a plain value stays a plain dict, as on the base. If a MAP target should still refuse{"key": ["a"], "value": "b"}with the clear message, decide that inTransformDictionaryToMapunder its MAP target rather than in the detector the analyzer, untyped parameters and STRUCT targets share. Add tests for those STRUCTs (and themap_entriesLIST of them) through identity map(),con.append, the row appender, a parameter and a DataFrame scan, and for{"key": [], "value": "ab"}. -
A declared STRUCT reads its dict cells through the untyped key and value guess, so the identity map() of a STRUCT with fields
keyandvaluecan return NULL for a row that is not NULL (src/numpy/numpy_scan.cpp:952).const auto shape = keyed ? KeyedShape(target, !implied_type) : LogicalType(LogicalTypeId::UNKNOWN);gives a STRUCT that holds no MAP and no timestamp the target UNKNOWN, so{"key": None, "value": None}(from{'key': NULL, 'value': NULL}::STRUCT(key VARCHAR, value VARCHAR)) and{"key": [1], "value": None}(from aSTRUCT(key INTEGER[], value INTEGER)) convert as a NULL MAP, which the kind check accepts as NULL: the identity map(), with and withoutschema=, returns[(None,)]where 2.0.0 and the merge base refused the column with a mismatch.con.appendalready read these as NULL on 2.0.0. Ask: read a dict for a declared STRUCT by the STRUCT's field names (a shape, or the declared type, for any type that holds a STRUCT), so the key and value guess applies only where no type is given; this also fixes the STRUCT side of finding 1 and thecon.appendcase. Add the identity test of such a STRUCT with both fields NULL and with a list beside NULL, and rerun the STRUCT rows of the benchmark table if the declared path changes.
Nits the author may ignore
- The comment above the rule (python_conversion.cpp:132 to 134) says "A list on either side ... makes the dict a MAP of the two lists", but two lists of different lengths still return false at line 150 and read as a plain dict; the comment should follow whatever rule finding 1 lands on.
IsMapComponentList(python_conversion.cpp:102) repeatsShapeTakesAsListexcept for arrays of no dimensions, andkey_none || value_noneon line 142 is redundant, since None is never a list.- No test sends an aware pandas Timestamp without nanoseconds, the
PyType_IsSubtypeside of the new aware branch (numpy_scan.cpp:1048), throughcon.append; my probes show it reads as on the base. - A DataFrame scan of
{"key": pd.Series([...]), "value": pd.Series([...])}now reads the dict's text as VARCHAR where the base raised "Unable to transform python value"; a rare input, mentioned only because it changes silently.
The description
The rewritten description checks out against the code with three small exceptions, besides the paragraph that finding 1 changes. "where on v2.0-cyanoptera those raised an internal error" for {"key": ["a"], "value": "b"} holds for con.append, the row appender and a parameter, but map() raised a cast error there. "con.append of aware datetimes or aware Timestamps into a TIMESTAMP column 13 to 23 percent fewer": I measure 23 percent for datetimes, 20 percent for Timestamps on pandas 2.3 and 11 percent (13.49 G against 12.06 G) for Timestamps on pandas 3.1. And a DataFrame scan or an untyped parameter now infers {"key": "x", "value": None} as a STRUCT("key" VARCHAR, "value" "NULL") where 2.0.0 inferred a NULL MAP; the description names only con.append, the row appender and a typed parameter. The four open behaviour changes (text columns mixing str with other scalars, objects without a DuckDB type, nested TIMESTAMP_NS and TIME_NS at microseconds, containers typed together) are listed as changes from 2.0.0, and the Timestamp.max change through con.append is listed and holds: infinity and -infinity into TIMESTAMP_NS, where the base stored 2262-04-11 23:47:16.854775 and refused the minimum, and microseconds into TIMESTAMP.
Outside findings I did not take
- Codex: the NumPy import lookup in
IsMapComponentListretried per value when NumPy is missing (the same lookup already runs for every unclassified object inGetPythonObjectType; not measured to matter); the NULL MAP reading ofMAP {'key': [1], 'value': NULL}pinned by the new test (df()hands that MAP over as exactly the dict that has meant a NULL MAP since 2.0.0, so one of the two readings must lose; the STRUCT side, which has no such ambiguity, is finding 2). - code-review: the positional read of
{"value": ..., "key": ...}(pre-existing on 2.0.0 and the base, a follow up); the comment "Verify that both the keys and values are of the same length" (predates this change). - Codex's and code-review's other findings (the list beside a plain value, the raw TypeError, the empty MAP, the UNKNOWN shape for a declared STRUCT) are findings 1 and 2.
What I verified, on 66a35e7 (range c0f203b..66a35e7, the upstream side of merge 891364f left out)
- Probes against Release builds of this head and of the merge base c0f203b, on pandas 3.1.1 and 2.3.3, and against the 2.0.0 wheel: the key and value cases the author lists (identities of
MAP {'key': 'ab', 'value': 'cd'},'cde', ints, a NULL value, a NULL key entry, both NULL and a list beside NULL;{"key": [..], "value": [..]}as lists, tuples and arrays, with None on either side and on both; a list beside a scalar;{"value": ..., "key": ...}), each through map() with and withoutschema=,con.append, the row appender, a typed and an untyped parameter and a DataFrame scan. The author's claims hold for MAP targets;{"value": [...], "key": [...]}behaves as on the base. STRUCTs with fieldskeyandvalue(findings 1 and 2) through the same callers and a Python UDF. - The aware TIMESTAMP fast path: 12 aware cells (fixed offsets, zoneinfo with fold, aware Timestamps with and without nanoseconds and of unit s, 9999-12-31, a datetime subclass, a tzinfo whose
utcoffsetreturns None or raises,datetime.maxwith an offset) throughcon.appendinto TIMESTAMP and TIMESTAMPTZ and through map() with and withoutschema=, in the America/New_York session zone, read exactly as on the base except thatschema=now casts them (the stated cast, as in round 1); the round 4 fast path equivalence probe gives the same results as on the base. - Benchmarks, instruction counts of the calling thread and wall time, Release, one thread, 1M rows, two rounds alternating base and head, pandas 3.1 and 2.3: aware datetimes into TIMESTAMP 8.00 G against 6.19 G (0.79 against 0.50 s), aware Timestamps 13.49 G against 12.06 G on pandas 3.1 and 8.07 G against 6.44 G on pandas 2.3, naive datetimes 4.82 G against 2.57 G; every round 2 and round 3 case (VARCHAR, UUID, BLOB, TIME, INTEGER[], SMALLINT[], TINYINT[], FLOAT[], STRUCT(x INTEGER), STRUCT(t TIMESTAMP), STRUCT(d DATE), BIGINT, an added column of None, TIMESTAMP through
astype(object)) andcon.appendof eight object column kinds are at or below the base, apart from BIGINT (+1 to 3 percent) and the column of None (+9 to 10 percent), as the description says. - The stable ABI: map.cpp, python_conversion.cpp, numpy_scan.cpp, numpy_result_conversion.cpp, pandas/bind.cpp and pyresult.cpp compile with the build's own command plus
-DPy_LIMITED_API=0x030C0000 -fsyntax-only, no errors. - Tests: test_map_types, test_map, test_map_lifetime, the appender tests, tests/fast/pandas, tests/fast/numpy and tests/fast/udf on pandas 3.1: 3511 passed, 132 xfailed; on pandas 2.3 the same 17 map lifetime tests fail as on the base and the rest pass; the far year test runs TIMESTAMP, TIMESTAMP_MS and TIMESTAMP_S for 1500, 2500 and 9999 and passes; this head's test_map_types.py against the base build: 180 of 330 fail, as the description says.
- CI at review time (00:00 UTC): code quality, submodule sanity and the cp312 seed wheels for Windows x86 64 and ARM64, manylinux aarch64 and macOS arm64 pass; the sdist, the manylinux x86_64 seed, the cp311 sanity jobs and the full API build with assertions are running and the macOS x86_64 seed is queued; nothing is red.
|
|
||
| // If either of the components is NULL, return early | ||
| if (nb::none().is(keys) || nb::none().is(values)) { | ||
| if (key_none || value_none || !key_list || !value_list) { |
There was a problem hiding this comment.
if (key_none || value_none || !key_list || !value_list) { returns true for a list beside a plain value, so {"key": "color", "value": ["red", "blue"]}, the cell df() gives for map_entries(MAP {'color': ['red', 'blue']}), is read as a MAP of two lists and refused: identity map() with and without schema=, con.append into STRUCT(key VARCHAR, value VARCHAR[])[], the row appender, a parameter and a DataFrame scan all fail, a scan of {"key": [1], "value": 5} raises a raw TypeError: object of type 'int' has no len(), and {"key": [], "value": "ab"} reads as an empty MAP through select ? and a scan. 2.0.0 and the merge base read all of these as STRUCTs, as the rule of 6746e39 did. Ask: take the key and value form only when both sides are lists, tuples or arrays of equal length, a list beside None, or None on both; keep any MAP-only refusal in TransformDictionaryToMap under its MAP target; add tests for these STRUCTs through every caller.
| TypeVisitor::Contains(target, LogicalTypeId::TIMESTAMP_MS) || | ||
| TypeVisitor::Contains(target, LogicalTypeId::TIMESTAMP_SEC)); | ||
| const auto bits_refused = bit_storage_refused && TypeVisitor::Contains(target, LogicalTypeId::BIT); | ||
| const auto shape = keyed ? KeyedShape(target, !implied_type) : LogicalType(LogicalTypeId::UNKNOWN); |
There was a problem hiding this comment.
A STRUCT that holds no MAP and no timestamp gets LogicalType(LogicalTypeId::UNKNOWN) here, so its dict cells go through the untyped key and value guess: the identity map() of {'key': NULL, 'value': NULL}::STRUCT(key VARCHAR, value VARCHAR) or of {'key': [1], 'value': NULL}::STRUCT(key INTEGER[], value INTEGER) returns [(None,)], with and without schema=, where 2.0.0 and the merge base refused the column. Ask: read a dict for a declared STRUCT by its field names (a shape or the declared type for any type that holds a STRUCT), and add these identities as tests.
…d STRUCT by its field names
…the appender's helper, a fine datetime64 is a TIMESTAMP_NS at every depth
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes. The premise still holds: on 2.0.0, map() hands ENUM columns back as TINYINT codes, refuses STRUCT and MAP identities and loses duplicate column names, and this head fixes all of them. Both round 5 findings are fixed, and the merge of the current v2.0-cyanoptera resolves 748's numpy scalar reading as intended. Two hot path costs must come down before merge. Both were measured in this round; the first has been in the branch since round 2 but no benchmark row covered it. The first is that con.append of plain dicts into a MAP column is about 3.5 times slower than on the base. The second is that a dict for a wide STRUCT is now matched to its fields in quadratic time. A test gap and the description also need fixing. The GitHub body is still the old "Merge after #8" text and must be replaced with the rewritten description before merge.
Findings, by severity
-
High, hot path:
con.appendof an object column of plain dicts into a MAP column is 3.4 to 3.6 times slower than on the base (200,000 rows of{"a": i, "b": 1}, one thread, Release; calling thread user space instructions and wall time, lower of two runs):into pandas 3, base pandas 3, head pandas 2, base pandas 2, head 2.0.0 MAP(VARCHAR, INTEGER) 5.47 G, 0.52 s 18.85 G, 2.04 s 5.48 G, 0.53 s 18.86 G, 2.08 s 3.51 G, 0.30 s MAP(VARCHAR, BIGINT) 5.48 G, 0.52 s 20.02 G, 2.31 s 5.47 G, 0.52 s 20.01 G, 2.33 s 3.51 G, 0.30 s MAP(VARCHAR, VARCHAR) 5.87 G, 0.57 s 19.32 G, 2.16 s 5.87 G, 0.57 s 19.32 G, 2.18 s 3.63 G, 0.31 s The base converts each dict without a target, as one STRUCT Value, and casts the whole vector into the MAP at once. The head gives a type holding a MAP a shape, so each cell builds its MAP in
MapFromEntries(src/native/python_conversion.cpp:394 to 434). That means one STRUCT Value per entry, thenValue::MAP(child_type, elements), which in the engine takes those STRUCTs apart and builds them again. Every child goes throughDefaultCastAs, which constructs a freshCastFunctionSeteven when the types already match (external/duckdb/src/common/types/value.cpp:789, 877 to 883, 2192 to 2196), and each key is hashed twice throughMapKeyCheck. Stack samples of the head during such an append show those frames throughout. That cause comes from reading the code plus the samples, not from a profiler. The key and value list form costs the same on both builds (21.15 G on the base, 18.94 G on the head), so the plain dict form now costs about what that form costs. Ask: bring the plain dict into a MAP column back to the base's cost or near it. One option is to build the MAP Value once from the converted keys and values (Value::MAP(key_type, value_type, keys, values), without the intermediate STRUCT Values), or to write the entries into the vector's children directly. Add these rows (pandas 2 and 3, base and head) to the benchmark table. -
Medium, hot path: since 0da3cbf every declared type holding a STRUCT takes a shape. A dict for a shaped STRUCT finds each key's field by a linear search over all fields (src/native/python_conversion.cpp:259 to 265), which is quadratic in the field count per cell.
con.appendof dicts into STRUCT(f0 INTEGER, ..., f99 INTEGER), 50,000 rows: 19.62 G, 1.88 s on the base, 53.84 G, 4.68 s on the head. With 200 fields and 20,000 rows: 15.45 G, 1.36 s against 67.32 G, 5.43 s. The map() identity of the same columns costs 46.8 against 65.9 G (+41 percent) and 37.7 against 76.9 G (+104 percent). At 20 fields the head is still 15 percent cheaper than the base. Ask: match fields in linear time, for instance with an index of the shape's field names built once per vector or by trying the field at the same position first, and add a 100 field row to the benchmark. -
Low, tests: the merge makes a datetime64 finer than a microsecond round to a stated coarser unit at depth, where the round 5 tree truncated. But
test_map_nested_timestamp_with_nanoseconds_for_a_stated_coarser_unit_reads_at_microseconds_as_at_the_topuses.000005123and.005123, which give the same result under both rules (tests/fast/test_map_types.py:974, 1001). The head does round:.000005789reads.000006at the top level and as a LIST element, STRUCT field and MAP value, andnp.datetime64(-1, "ns")reads the epoch. A pandas Timestamp at the same places reads.000005and23:59:59.999999, as at the top level. Ask: add a value that rounds up and a negative one for both datetime64 and Timestamp, so the test pins the rule that the merge changed. -
Description (not in the code): the GitHub body must be replaced with the rewritten description. Round 5's three corrections are in it: the base's map() refusal is a cast error, the aware Timestamp gain is given per pandas version, and the STRUCT inference of
{"key": "x", "value": None}is listed. The four changes from 2.0.0 that wait on a maintainer's choice are listed. Two additions are needed. The benchmark table must carry the rows of findings 1 and 2. Andcon.appendof a list beside a plain value of another length into a MAP column must be listed as a change:{"key": ["a"], "value": "bb"}and{"key": [1, 2], "value": 5}stored the two entries{key='[a]', value=bb}on 2.0.0 and on the base and are now refused. The description mentions only the equal length case, which the base turned into an internal error.
Nits (optional)
- A numpy array of no dimensions holding a pandas Timestamp with nanoseconds loses them:
DeclaredValue(src/numpy/numpy_scan.cpp:328 to 343) checksHoldsNanosecondsbefore unwrapping, socon.appendinto TIMESTAMP_NS stores2020-01-01 00:00:00and an inferred TIMESTAMP accepts it. 2.0.0 and the base raised a TypeError there, and nobody puts such a cell in a frame. - For a MAP column the row appender and a query parameter read a list beside a plain value as two entries, while
con.appendand map() refuse it. The refusal is loud and the input rare.
Outside findings rejected
- Codex: remove the refusal of a list beside a plain value for a MAP target. The refusal is loud and clear for an ambiguous dict nobody writes on purpose, so it is kept, and the description must list it (finding 4).
- Codex: a numpy datetime64[ns] array as a MAP key or value list, or as a LIST, reads through
tolist()as integers. The same happens on the base and on 2.0.0, and the description states it. - Codex: an array of no dimensions holding a Timestamp. Confirmed, but graded a nit (above).
- Codex questions on STRUCT dicts with fewer, extra, reordered, disjoint or no fields: probed through
con.appendand map() with and without a schema. They follow the stated rule (by name, a missing field NULL, an unknown field refused without a schema, the engine's cast with one). No defect. - Codex's quadratic STRUCT lookup and test gap are findings 2 and 3.
- The code-review skill's findings for this round were not saved in a readable form, so none are triaged here.
What was verified (head ca7f935, range 5f489fb..ca7f935)
- Round 5 probes on head, base and 2.0.0. STRUCTs with fields key and value, one a list, and the
map_entriesoutput read as their STRUCT through map() with and without a schema,con.append, the row appender, parameters and a scan. There is no raw TypeError and no empty MAP. STRUCT rows with NULL fields round trip. The MAP cases kept in round 5 still hold, and no probe left a database unusable. The fast path equivalence probe matches the base apart from the intended refusals. - The merge's conflict resolution (git show with the remerge diff option on ca7f935):
DeclaredValueandTransformTimestampnow read every numpy scalar through 748'sPythonValueOfNumpyScalar, and the PR's own scalar helpers are gone. A fine datetime64 rounds at depth as at the top level, throughcon.appendand map() with a schema. An inferred type refuses it. 748's appender code is untouched by the range. - Limited API (Py_LIMITED_API 3.12) syntax compile of the eight touched C++ files: 0 errors.
- Benchmarks of head against a Release build of the base 5f489fb, one thread, pandas 2 and 3: the round 5 table rows hold (STRUCT identities 10 to 17 percent cheaper, aware datetimes 22 percent and aware Timestamps 11 percent (pandas 3) and 20 percent (pandas 2) cheaper), apart from findings 1 and 2.
- Tests: the touched files (map, map types, map lifetime, appender, pandas, numpy, udf) on pandas 3: 3602 passed, 34 skipped, 132 xfailed; on pandas 2, 3519 passed and 47 failed, the same 47 that fail on the base (17 map lifetime tests and 30 dictionary string tests that need the pandas and pyarrow versions their skip reason names).
- CI on ca7f935: 30 checks pass, 1 skipped matrix placeholder, nothing red.
| @@ -337,9 +434,35 @@ Value TransformStructFormatDictionaryToMap(optional_ptr<ClientContext> context, | |||
| return Value::MAP(ListType::GetChildType(map_type), std::move(elements)); | |||
There was a problem hiding this comment.
return Value::MAP(ListType::GetChildType(map_type), std::move(elements));Every dict cell for a MAP column now builds one STRUCT Value per entry here, and the engine's Value::MAP(child_type, values) takes them apart and builds them again, each child through DefaultCastAs, which constructs a fresh CastFunctionSet, and each key hashed twice. con.append of 200,000 two entry dicts into MAP(VARCHAR, INTEGER) costs 18.85 G instructions and 2.04 s here against 5.47 G and 0.52 s on the base, which reads the dict as a STRUCT and casts the vector once (pandas 2 the same). Please bring this back near the base's cost, for instance by building the MAP once from the converted keys and values with Value::MAP(key_type, value_type, keys, values) or by writing the entries into the vector's children, and add the row to the benchmark table.
| auto val = | ||
| TransformPythonValue(context, dict.values.attr("__getitem__")(i), LogicalType::UNKNOWN, nan_as_null); | ||
| LogicalType child_type = LogicalType::UNKNOWN; | ||
| for (idx_t child = 0; shape_only && child < StructType::GetChildCount(target_type); child++) { |
There was a problem hiding this comment.
for (idx_t child = 0; shape_only && child < StructType::GetChildCount(target_type); child++) {Since every declared STRUCT takes a shape, each key of each dict searches all fields, which is quadratic in the field count per cell: con.append into a 100 field STRUCT, 50,000 rows, 19.62 G and 1.88 s on the base against 53.84 G and 4.68 s here; 200 fields 15.45 G against 67.32 G; the map() identity +41 and +104 percent. Please match fields in linear time (an index of the shape's names built once per vector, or the field at the same position tried first) and add a wide STRUCT row to the benchmark.
| mapped.fetchall() | ||
|
|
||
|
|
||
| FIVE_MICROSECONDS = np.datetime64("2020-01-01T00:00:00.000005123", "ns") |
There was a problem hiding this comment.
FIVE_MICROSECONDS = np.datetime64("2020-01-01T00:00:00.000005123", "ns").000005123 (and .005123 at line 1001) read the same under truncation and under the engine's rounding, so this test does not pin the rule the merge changed. The head rounds: .000005789 reads .000006 at the top and as a LIST element, STRUCT field and MAP value, and np.datetime64(-1, "ns") reads the epoch, while a pandas Timestamp reads .000005 and 23:59:59.999999. Please add a value that rounds up and a negative one, for datetime64 and for Timestamp.
…CT shape's fields once per scan
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes. The round 6 asks are met: con.append of 200,000 two entry dicts into a MAP column now takes 23 to 25 percent fewer instructions than the merge base 5f489fb (it took 3.4 to 3.6 times as many), dicts into STRUCTs of 100 and 200 fields take 7 percent fewer in field order and 2 percent fewer shuffled, and the new rounding tests tell rounding from truncation. Two of the new fast paths have defects that must be fixed before merge: the per thread STRUCT field index can be freed under a conversion that is still using it, which crashes the process, and the MAP fast path writes an OrderedDict in its storage order instead of its iteration order. CI is green on fee081e. The GitHub body is still the old "Merge after #8" text and must be replaced with the rewritten description, with the additions below.
1. Medium, crash (use after free): a nested declared scan clears the field index an outer conversion holds. src/numpy/numpy_scan.cpp:946 to 950 clears the whole thread local index at the end of every declared scan:
struct FieldIndexesOfTheScan {
~FieldIndexesOfTheScan() {
ClearStructFieldIndexes();
}
} field_indexes;while TransformDictionaryToStruct keeps a pointer into it across the conversion of every later field (src/native/python_conversion.cpp:301 to 314, fields = &FieldIndexOf(target_type); then TransformPythonValue(...)). Converting a value can run Python (a tzinfo.utcoffset, for one), and if that Python appends a DataFrame on another connection, the inner scan's guard frees the outer scan's index. Reproducer: con.append of 20 dicts into STRUCT(t TIMESTAMPTZ, f0 INTEGER, ..., f29 INTEGER), keys in reverse field order, t an aware datetime whose utcoffset runs other.append("y", pd.DataFrame({"c": pd.Series([1, 2, 3], dtype=object)})): the head dies with SIGSEGV (exit 139), the merge base stores the 20 rows; the same dicts in field order, which never build the index, run on the head too. The reach is narrow, but it is memory corruption this change introduces. Ask: let only the outermost declared scan on a thread clear the index (a depth count, or a guard that saves and restores the thread's map), or own the index in the scan and pass it down, and add the reproducer as a test.
2. Medium, wrong result: the MAP fast path reads a dict subclass's storage, not its iteration order. src/numpy/numpy_scan.cpp:1073 admits subclasses with if (!PyDict_Check(cell.ptr())) { and line 1087 walks them with PyDict_Next. After od = OrderedDict([("a", 1), ("b", 2), ("c", 3)]); od.move_to_end("a"), con.append into a MAP(VARCHAR, INTEGER) column stores {a=1, b=2, c=3} (map_keys gives ['a', 'b', 'c']); 2.0.0, the merge base, the head's own general reader (the same dict into MAP(VARCHAR, INTEGER[])), a query parameter and a DataFrame scan all store {b=2, c=3, a=1}. MAP entry order is visible (map_keys, map_values, list functions, the dict fetchall returns). Ask: take the fast path only for PyDict_CheckExact, send subclasses through the general reader, and test an OrderedDict after move_to_end with map_keys for con.append and map() with schema=.
3. Low, test: the out of order STRUCT test would pass without the field index. tests/fast/pandas/test_append_df.py:589 uses only INTEGER fields (STRUCT(a{i} INTEGER, b{i} INTEGER, c{i} INTEGER)), and KeyedShape gives an INTEGER field no shape (src/numpy/numpy_scan.cpp:377, default: return LogicalType::UNKNOWN;), so a key the lookup missed converts exactly as a found one and the engine's cast then matches the names. Its comment, "more than the per thread field indexes keep", describes a limit the code does not have. Ask: give the reordered dicts fields whose reading depends on their type, such as a pandas Timestamp with nanoseconds for a TIMESTAMP_NS field and a MAP field keyed by BLOB, also inside a LIST, and fix the comment.
4. Low, silent loss: a zone aware Timestamp with nanoseconds loses them in a TIMESTAMPTZ_NS field or element. DeclaredValue keeps nanoseconds only for naive Timestamps (src/numpy/numpy_scan.cpp:315, auto naive_timestamp = target.id() == LogicalTypeId::TIMESTAMP_NS || ...), and the shape treats TIMESTAMPTZ as fitting TIMESTAMPTZ_NS, so map() of {'t': '2020-01-01 00:00:00.000000001+00'::TIMESTAMPTZ_NS} handing back {"t": pd.Timestamp("2020-01-01 00:00:00.000000001", tz="UTC")} reads 2020-01-01 00:00:00+00, with and without a schema and in a LIST, where the merge base refused it; the naive TIMESTAMP_NS case keeps its nanosecond. con.append drops it on the base and 2.0.0 as well. Ask: keep the nanoseconds of an aware Timestamp for a TIMESTAMPTZ_NS target as for a naive one, or refuse them, and test it.
5. Description. Replace the GitHub body with the rewritten text, which I checked against the code and my runs: its MAP and STRUCT benchmark rows match mine (MAP appends 23 to 25 percent fewer instructions, 100 field STRUCT 7 percent fewer), its fast path paragraph matches the code, and the four changes from 2.0.0 that wait on a maintainer decision are listed as behaviour changes. Add one unlisted change: con.append of a dict with a NaN key into a MAP column, {float("nan"): 1} into MAP(DOUBLE, INTEGER) or MAP(VARCHAR, INTEGER), now raises "Map keys can not be NULL." where 2.0.0 and the merge base stored nan (and 'nan'); a query parameter still stores it. The key and value list form already raised on both, so the new behaviour is the consistent one, but it is a change for a function that worked.
Nits, optional. The NULL key test (test_append_df.py:637) accepts any duckdb.Error; pin InvalidInputException and "Map keys can not be NULL.". The mixed row MAP test stays below one child vector of entries per batch and compares dicts, which ignore entry order; my 5,000 row probe with long keys, NULL values and other cells interleaved matched the merge base, so this is coverage only. con.append of an object column of numpy datetime64 scalars into TIMESTAMP_NS takes 1.6 to 2.5 percent more instructions (also seen in round 6), worth a line in the benchmark text.
Outside findings not taken. From the code-review skill: a MAP of a BIGINT beside a float silently rounded under an inferred type (my probe: refused, "got [MAP(VARCHAR, DOUBLE)]"); {'value': [...], 'key': [...]} read by position (the same on the merge base and 2.0.0 for parameters and con.append); the deleted NULL_typed_Value_for_BIGINT case (duckdb.Value(True, SQLNULL) now reads 1 in map(), as a query parameter and con.append read it on the base too; pinning the new reading would be a nice addition); a raw pandas error for two returned columns of one label (not reproduced, the frame maps cleanly); a worse message for an unconvertible value beside a key list (fails loudly anyway); repeated walkers, the shape alias and comment length (maintenance, not defects; the last cites no rule of this repository). From Codex: Codex, dict subclasses overriding keys() or values(): adversarial, and the exact dict check in finding 2 removes it anyway. Codex, a dict mutated by Python while its entries convert: no ordinary reproducer. Codex, MapFromEntries equivalence for heterogeneous keys: probes of keys that collide after their cast (1.4 and 1.2 into INTEGER, 1 and '1' into VARCHAR) raise the engine's "Map keys must be unique." on both readers. Codex, duplicate target field names: the engine refuses such STRUCT types.
Verified. Head fee081e, range 5f489fb..fee081e (the merge of the base excluded). The code-review skill (high effort, this range) and two Codex reviews were triaged with probes; its use after free finding is finding 1. Benchmarks, Release builds of the head and the merge base, one thread, two alternating rounds, pandas 3.1 and 2.3, instruction counts from perf_event_open: MAP appends (MAP(VARCHAR, INTEGER) 5.47 G to 4.12 G, 0.52 s to 0.39 s; key and value lists 21.2 G to 12.8 G), wide STRUCT appends in field order and shuffled at 20, 100 and 200 fields, the identity maps of round 2 to 6 (no new regression; the BIGINT identity +1 to 3 percent and the column of None +10 to 12 percent are described), aware datetimes and Timestamps. Probes against the head, the merge base and the 2.0.0 wheel: NULL cells, empty dicts, None and NaN keys, keys colliding after their cast, nested MAPs, MAPs in STRUCTs and LISTs, dicts mixed with MAP text and key and value lists in one column, 5,000 rows with long keys, a conversion error mid vector; STRUCT rows with missing, extra, reordered and case folded fields (con.append as on the base; map() with schema= through the engine's cast, as described), nested and inside LISTs and MAPs, 4,100 shuffled rows of 60 fields. Limited API compile of the eight touched C++ files with Py_LIMITED_API=0x030C0000: no errors. pytest on pandas 3 over test_map_types, test_map_lifetime, test_map, the appender tests, tests/fast/pandas, numpy and udf: 3639 passed, 34 skipped, 132 xfailed.
| bool bit_storage_refused) { | ||
| DeclaredVector staged(count); | ||
| struct FieldIndexesOfTheScan { | ||
| ~FieldIndexesOfTheScan() { |
There was a problem hiding this comment.
Every declared scan clears the whole thread local index when it ends, including one nested inside another scan's conversion:
~FieldIndexesOfTheScan() {
ClearStructFieldIndexes();
}TransformDictionaryToStruct keeps fields = &FieldIndexOf(target_type); (python_conversion.cpp:302) across the conversion of every later field, and a conversion can run Python. A tzinfo.utcoffset that appends an object column on another connection, met while appending dicts with keys out of field order into STRUCT(t TIMESTAMPTZ, f0 INTEGER, ..., f29 INTEGER), crashes the head with SIGSEGV; the merge base stores the rows, and the head does too when the keys are in field order. Please let only the outermost scan on the thread clear the index (a depth count, or save and restore), or own the index in the scan, and add the reproducer as a test.
| vector<Value> keys; | ||
| vector<Value> values; | ||
| ScanObjectCells(context, col, stride, masked, count, offset, out, [&](nb::handle cell, idx_t row) { | ||
| if (!PyDict_Check(cell.ptr())) { |
There was a problem hiding this comment.
PyDict_Check admits dict subclasses, and PyDict_Next below walks their storage, not their iteration order. After od.move_to_end("a") on OrderedDict([("a", 1), ("b", 2), ("c", 3)]), con.append into MAP(VARCHAR, INTEGER) stores {a=1, b=2, c=3}, while 2.0.0, the merge base, this branch's general reader (MAP(VARCHAR, INTEGER[])), a query parameter and a DataFrame scan store {b=2, c=3, a=1}. Please use PyDict_CheckExact here so subclasses take read(cell, row), and test map_keys of such an OrderedDict through con.append and map() with schema=.
|
|
||
| def test_struct_dicts_with_keys_out_of_field_order_read_each_field_by_name(self, duckdb_cursor): | ||
| # Twelve STRUCT types, more than the per thread field indexes keep, each dict's keys in reverse field order | ||
| inner = [f"s{i} STRUCT(a{i} INTEGER, b{i} INTEGER, c{i} INTEGER)" for i in range(12)] |
There was a problem hiding this comment.
With only INTEGER fields, a key the field lookup misses converts exactly as a found one (KeyedShape gives INTEGER no shape) and the engine's cast then matches the names, so this test passes with or without the index. Please add reordered fields whose reading depends on their type, for example a nanosecond pd.Timestamp into a TIMESTAMP_NS field and a MAP field keyed by BLOB, also inside a LIST. The comment's "more than the per thread field indexes keep" describes a limit the code does not have.
… dicts in the MAP reader, keep a zoned Timestamp's nanoseconds for TIMESTAMPTZ_NS
…seconds under a New York zone
el-marvin
left a comment
There was a problem hiding this comment.
Requesting changes, for one test only. Every round 7 finding is fixed in the code: the two crash reproducers (a tzinfo.utcoffset that appends a frame on another connection while dicts with keys out of field order convert into STRUCT(t TIMESTAMPTZ, f0 INTEGER, ..., f29 INTEGER)) now store all 20 rows on the head where the previous head died with SIGSEGV; an OrderedDict after move_to_end stores its iteration order as a MAP; a zone aware Timestamp keeps its nanoseconds in TIMESTAMPTZ_NS at every depth; and the reordered STRUCT test now depends on the field index. The one thing that must change is the new regression test for the crash, which with the default thread count seldom runs the inner scan on the thread that holds the index, so it may pass on the old code too. The GitHub body is still the old "Merge after #8" text and must be replaced with the rewritten description before merge, with the one correction below. CI on 5ee6ecc had 11 checks passed and none failed when the head moved. A test only commit, e4bb2a7, arrived on the branch while this review was being written; from its diff on GitHub it makes exactly the change finding 1 asks for (a one thread inner connection, closed, and a TIMESTAMP_NS and a MAP(BLOB, INTEGER) field asserted after the callback) and adds a New York zone to the zoned nanosecond tests. I have not run it; the next round will, and if it holds, only the description items below remain.
1. Low, test: the reentrancy test seldom nests the two scans on one thread. tests/fast/pandas/test_append_df.py:613 opens the inner connection with the default thread count:
other = duckdb.connect()so the inner con.append scan usually runs on one of that database's own worker threads, whose thread local field index is a different one. Recording threading.get_native_id() in a tzinfo of the inner frame, in this test's exact scenario repeated 20 times, 120 of 1200 inner conversions ran on the outer scan's thread; with duckdb.connect(config={"threads": 1}) all 1200 did. The round 7 reproducer, which crashed a release build of the old code, opened its inner connection with one thread. And the fields converted after the callback are all INTEGER (**dict.fromkeys(reversed(fields[:-1]), row)), which KeyedShape gives no shape, so a lookup that lost its index would store the same rows. Ask: open other with config={"threads": 1} (and close it), and give the dict a field after the callback whose reading depends on its type, such as a TIMESTAMP_NS field given a Timestamp with nanoseconds or a MAP(BLOB, INTEGER) field, and assert its value.
2. Description. Replace the GitHub body with the rewritten text. I checked the sentences added since round 7 against the code and my runs: the exact dict rule of the MAP fast path, the NaN key now refused, the zone aware Timestamp.max and .min stored as infinity and -infinity (the merge base stored the finite 2262-04-11 23:47:16.854775+00 and refused the minimum, and a datetime64[ns, UTC] column of the frame already stores infinity on the base), the inferred TIMESTAMPTZ refusing a zoned Timestamp with nanoseconds at the top level, as a field and as a list element with schema= restoring the microsecond reading, and the new test sentences. The four changes from 2.0.0 that wait on a maintainer decision (mixed text refused, an object without a DuckDB type refused in an inferred text column, nested TIMESTAMP_NS and TIME_NS identities at microseconds, containers read as the conversion types them) are still listed as behaviour changes. One sentence is wrong: "an inferred TIMESTAMPTZ refuses them, where map() read a TIMESTAMPTZ_NS field handed back as such a Timestamp at microseconds". On the merge base map() refused that Timestamp for a TIMESTAMPTZ_NS column at the top level, as a field and as a list element, with and without a schema ("UDF column type mismatch, expected [STRUCT(t TIMESTAMPTZ_NS)]"); it was con.append that stored it at microseconds, as 2.0.0 does. Please say that instead. And add the TIMESTAMPTZ_NS row from the benchmarks below.
Benchmarks, no regression. Release builds of this head and of the merge base 5f489fb, one thread, two alternating rounds, pandas 3.1 and 2.3, instruction counts from perf_event_open (wall times move with them). The fast paths this commit touched hold their round 7 gains: con.append of 200,000 two entry dicts into MAP(VARCHAR, INTEGER) 5.47 G to 4.12 G instructions (0.53 s to 0.40 s), key and value lists 21.1 G to 12.8 G; dicts into STRUCTs of 20, 100 and 200 fields 6 to 7 percent fewer in field order and 1.6 to 1.9 percent fewer shuffled; the map() identities of those STRUCTs 32 to 37 percent fewer. The zoned paths: con.append of 300,000 aware datetimes into TIMESTAMPTZ[] 3.87 G to 3.86 G and into TIMESTAMPTZ 2.02 G to 2.01 G, into STRUCT(t TIMESTAMPTZ) 1.3 percent more, the identity of STRUCT(t TIMESTAMPTZ) 8.5 to 8.7 percent fewer, all as the description says; zone aware Timestamps with nanoseconds into TIMESTAMPTZ_NS 74 to 79 percent fewer, since they now skip the datetime conversion. The earlier rows (the identities of BLOB, TIME, UUID, lists, STRUCTs, the appends of plain object columns, aware datetimes into TIMESTAMP, numpy datetime64 scalars) match round 7 within half a percent. One small new cost is not in the description: con.append of an object column of zone aware Timestamps without nanoseconds into TIMESTAMPTZ_NS takes 1.6 percent more instructions on pandas 3 and 5.6 percent more on pandas 2 (2.05 G to 2.17 G for 300,000 cells), and of aware datetimes 1.2 percent more, from the nanosecond test each cell now meets, as the naive pair pays for TIMESTAMP_NS; please add it to the benchmark paragraph.
Nits, optional.
- A numpy datetime64 finer than a microsecond, nested under a stated TIMESTAMPTZ as a STRUCT field or a LIST element, still fails with "Conversion Error: Unimplemented type for cast (BIGINT -> TIMESTAMP WITH TIME ZONE)" through
con.append, while the same cell now reads at the top level, as a MAP(VARCHAR, TIMESTAMPTZ) value, in STRUCT(t TIMESTAMP) and in STRUCT(t TIMESTAMPTZ_NS). The merge base and 2.0.0 fail the same way, so this is a gap, not a regression; it comes fromKeyedShapegiving a stated TIMESTAMP_TZ no shape (src/numpy/numpy_scan.cpp:383to 385). - The zoned tests use only
tz="UTC"with a UTC session, so a conversion from the wall time instead ofTimestamp.valuewould pass them. My probe shows the right instants for Europe/Amsterdam, America/New_York through zoneinfo, a fixed +05:30 offset, dateutil's Europe/London and a DST fold; one non UTC zone in the TIMESTAMPTZ_NS test would pin it. - An aware Timestamp with nanoseconds into TIMESTAMP_NS, and a naive one into TIMESTAMPTZ_NS, still store no nanoseconds through
con.append, as on the merge base and 2.0.0, while the engine's cast between those types keeps them.
Outside findings not taken. From the code-review skill: the BY_VALUE verdict between TIMESTAMPTZ and TIMESTAMPTZ_NS refusing an aware datetime of the year 3000 for an inferred TIMESTAMPTZ_NS (it failed in the cast before, so it fails loudly either way); Timestamp.max becoming infinity (listed in the description and the same as the frame's typed column scan); the nanosecond rule written in four places, the zoned unit rule beside NaiveUnit, and eight TypeVisitor::Contains walks per vector (maintenance, not defects; the walks are per vector, not per cell); one shared thread local index that inner scans add to until the outer vector ends (bounded by that vector; 3,500 appends of 200 field dicts after the exception cases left the peak memory flat apart from the table's data) and a user type aliased duckdb_py_shape reaching the index outside a scope (adversarial). Its nested datetime64 and cross kind findings are the first and third nits, its UTC only finding the second, and its reentrancy test finding is finding 1. From Codex: the first review found the scope counter balanced under exceptions and asked for the typed field after the callback (in finding 1); its questions on Counter and defaultdict cells are answered by the code, which reads them through the general reader as the merge base read every dict. The second review's UTC only point is the second nit; its performance table raises no regression my measurements show.
Verified. Head 5ee6ecc, range 5f489fb..5ee6ecc with the merge of the base excluded; the new commit is 5ee6ecc. The round 7 reproducers on the head: probes/r7/p32.py (1 plain, 1, 3) and probes/r7b/p34.py (inorder, reversed) exit 0 with 20 rows and the right sums; p34 other gives {b=2, c=3, a=1} for the moved OrderedDict through con.append, matching 2.0.0, the merge base, a query parameter and a DataFrame scan. The depth count: a nested append whose conversion raises and is caught, a nested STRUCT append with keys out of order that raises and is caught, a nested STRUCT append that succeeds, a nested map() on an eight thread connection, an outer conversion that raises, then a plain append again, and four Python threads appending at once with nested appends: every head result exact (3,000 rows of 30 reordered TIMESTAMP_NS fields with nanoseconds and a MAP(BLOB, INTEGER) field). Reading from the code: the counter only increments in the constructor and decrements in the destructor of a scope on the stack, so unwinding keeps it right; shapes are built only inside ScanDeclaredColumn, after the scope opens; an unordered_map keeps its nodes in place when it rehashes, and each entry holds its type, so the pointer key cannot be reused while it is kept; a scan on another thread uses that thread's own index. The reordered STRUCT test fails without the index: the outer dict's keys l, s11, ..., s0 never sit at their field's position, so each would convert without a shape and its TIMESTAMP_NS field lose the nanosecond the test asserts, and inside each STRUCT the key m sits at position 0, where field a is, so its MAP(BLOB, INTEGER) value would convert as a STRUCT named by the bytes' text. Zoned Timestamps through con.append and map() against the merge base and 2.0.0 in TIMESTAMPTZ_NS, TIMESTAMPTZ, TIMESTAMP_NS, fields, lists and MAP values, for six zone kinds, without nanoseconds, aware datetimes, naive Timestamps and the extremes. Limited API: all touched C++ files compile with -DPy_LIMITED_API=0x030C0000, no errors. pytest on pandas 3 over test_map_types, test_map_lifetime, test_map, the appender tests, tests/fast/pandas, numpy and udf: 3669 passed, 34 skipped, 132 xfailed; on pandas 2.3 test_map_types, test_map and test_append_df: 656 passed, 2 skipped, 2 xfailed. The code-review skill (xhigh, this range) and two Codex reviews (the field index and MAP admission; the zoned timestamps) were triaged with probes.
| def test_struct_append_inside_a_conversion_keeps_the_outer_scans_field_index(self, duckdb_cursor): | ||
| # A tzinfo's utcoffset that appends on another connection runs a declared scan inside this one's, between | ||
| # this dict's out of order keys | ||
| other = duckdb.connect() |
There was a problem hiding this comment.
With the default thread count the inner append's scan usually runs on one of other's own worker threads, which have their own thread local index:
other = duckdb.connect()In this scenario repeated 20 times, 120 of 1200 inner conversions ran on the outer scan's thread; with config={"threads": 1} all 1200 did. The round 7 reproducer that crashed the old code used a one thread inner connection. The fields after the callback are also INTEGER, which take no shape, so a lost lookup stores the same rows. Please open other with config={"threads": 1} (and close it), and add a field after the callback whose reading depends on its type, such as a TIMESTAMP_NS given a Timestamp with nanoseconds or a MAP(BLOB, INTEGER), and assert its value.
el-marvin
left a comment
There was a problem hiding this comment.
This is ready to merge. The new commit, e4bb2a7, changes only tests/fast/pandas/test_append_df.py and tests/fast/test_map_types.py, and it makes exactly the test change round 8 asked for: the reentrancy test now opens its inner connection with one thread, so the inner con.append scans on the thread that holds the outer scan's STRUCT field index in every run, and it asserts a TIMESTAMP_NS field and a MAP(BLOB, INTEGER) field read after the callback, which only a working index stores right. The C++ is the code round 8 approved on 5ee6ecc. Two things stay open and are not code: the GitHub body is still the old "Merge after #8" text and must be replaced with the rewritten description before merge, and the four changes from 2.0.0 that the description lists as behaviour a function that worked on 2.0.0 meets (mixed str and other scalars in an inferred text column refused, an object without a DuckDB type in an inferred text column refused, nested TIMESTAMP_NS and TIME_NS identities round tripping at microseconds, and container elements typed together) remain maintainer decisions, as in every earlier round.
Findings. None that must change.
Description, before it is pasted. The corrected sentence now says that on v2.0-cyanoptera map() refused a zone aware Timestamp with nanoseconds for a TIMESTAMPTZ_NS column at every depth, with and without a schema, and that con.append stored it at microseconds, as 2.0.0 does; that matches my runs of a Release build of the merge base (the type mismatch at the top level, as a field and as a list element, inferred and with a schema; con.append storing the instant without its nanoseconds at each place). The benchmark paragraph now carries the TIMESTAMPTZ_NS numbers (zone aware Timestamps with nanoseconds 74 to 79 percent fewer instructions; without nanoseconds 1.6 percent more on pandas 3 and 5.6 percent more on pandas 2, 2.05 against 2.17 G instructions; aware datetimes 1.2 percent more), which match my round 8 measurements. One count is stale and can be fixed when pasting: "Of the 417 tests, 216 fail on v2.0-cyanoptera (5f489fb); the 201 that pass" should now read 423 tests and 222 failing, since the six New York cases of test_map_zoned_timestamp_ns_value_keeps_its_nanoseconds are new and all fail on the merge base (I ran this head's test_map_types.py against the merge base build: 222 failed, 201 passed).
Nits the author may ignore. other.close() at test_append_df.py:643 runs only when the assertions pass, so a failing run leaves the inner connection to the garbage collector; a with duckdb.connect(config={"threads": 1}) as other: block would close it on every path.
Outside findings not taken. From Codex (high effort, on the diff since round 8): only the connection close above, kept as a nit; its question whether the test fails on the old code is answered by the reasoning under Verified and the author's 6 of 6 crashes. From the code-review skill (medium effort, the whole range), each checked against 2.0.0, the merge base and the head with a probe: a dict of value before key reads its lists by position ({10: 'a', 20: 'b'}), the same on all three builds, so pre-existing and a possible follow-up; pandas Series, range and array.array as key and value lists no longer reading as a MAP, not reproduced (all three builds raise "Unable to transform python value"); {'a': 1, 'A': 2} into STRUCT(a INTEGER, b INTEGER) storing {'a': 1, 'b': NULL}, the same on the merge base; nanoseconds lost between a naive Timestamp and TIMESTAMPTZ_NS or an aware one and TIMESTAMP_NS, the round 8 nit, as on the base; a less specific mismatch message for a column untyped at bind that fails loudly anyway; per cell field name search and per vector shape rebuilds, with no regression measured (wide STRUCT appends 6 to 7 percent fewer instructions); duplicated integer range tables, maintenance; and comment style, citing rules that are not the repository's.
Verified. Head e4bb2a7, range 5f489fb..e4bb2a7, the merge of the base excluded. git diff 5ee6ecc..e4bb2a7 touches the two test files only (30 lines added, 12 removed). The reentrancy test passed in 12 of 12 separate runs on a Release build of the head. A probe in the test's new shape, recording the native thread of every conversion the inner scan makes, counted 1200 of 1200 inner conversions on the outer scan's thread with the one thread inner connection, against 102 of 1200 with the default thread count, and stored the right 20 rows in all 20 repetitions of each. On the code before 5ee6ecc, every declared scan cleared the thread's whole index when it ended, so with the inner scan always on the outer thread the outer conversion of n and m reads an index that has been freed; that is the crash round 7 reproduced, and I did not rebuild that code to rerun it. test_append_df.py and test_map_types.py: 577 passed on pandas 3.1.1 and 577 passed on pandas 2.3.3; the zoned nanosecond tests (UTC and America/New_York, with and without a schema, and through con.append) pass on both, and the 12 map() cases fail on the merge base. CI on e4bb2a7 at 08:21 UTC: 9 checks passed, among them the macOS, Windows and Linux seed builds with tests and the code quality checks, 20 still running, none failed.
Merge after #8 (
fix-map-udf-values); two of its test expectations change here, as described below.DuckDBPyRelation.map()does not return its input's types for many columns, even when the function returns its input unchanged. An ENUM column comes back as TINYINT, with a NULL turned into -1, and several nested and specialised types fail with a type mismatch:The same mismatch hits
MAP(INTEGER, INTEGER)(back asMAP(VARCHAR, INTEGER)),MAP(VARCHAR, BIGINT), a STRUCT whose field is NULL across a whole 2048 row chunk (STRUCT(a "NULL")), a fixed size ARRAY (back as a LIST), a UNION (back as its member's type), TIME_NS (back as TIME), and a MAP inside a STRUCT. A relation with two columns of one name loses one of them silently:con.values([1, 2]).project("col0 as a, col1 as a").map(lambda df: df)returns[(2,)]. Every case behaves the same on 1.4.1, 2.0.0, v2.0-cyanoptera and main.map() types its output twice and from two different things. At bind it calls the function with an empty frame built from the relation's types and types the returned frame with
Pandas::Bind; at execution it types every chunk's returned frame again withPandas::Bind, from the values, and requires both to be equal. An object column carries no type of its own, only values. On the empty bind frame it has no values and binds as NULL, andOverrideNullTypetakes the input column's type only when the frame has as many columns as the input and the names match position by position. At execution the analyzer guesses the type from a sample of the values: a Python int is INTEGER, a dict with int keys is a VARCHAR keyed map, a field that is NULL in every sampled row is NULL, a Python list is a LIST. Whatever the guess loses, the type check then refuses.Two more causes sit on the way into the function.
FunctionCallbuilds the frame from the arrays ofNumpyResultConversionand skips the stepDuckDBPyResult::FillNumpyapplies fordf(), so the function sees an enum's int8 dictionary codes, with -1 for NULL; the returned int8 column binds as TINYINT, and comparing the column with its text (df.e == 'b') never matches. And it builds the frame from a dict keyed by column name, so of two input columns of one name only the last survives; SQL inputs arrive deduplicated (a,a_1), the relationalprojectdoes not.Now one rule reads each returned column back. A column whose dtype carries a DuckDB type (numpy, a pandas extension dtype, a categorical, an Arrow backed column) must still bind to the type map() fixed for it at bind, exactly as before. An object column's type comes from one of three places: the schema; the input column of the same name, when the function got that input as an object column as well (VARCHAR, BLOB, UUID, LIST, STRUCT, MAP, UNION, ARRAY and the like); or, as a guess, the input column of the same name when that input is typed, such as a BIGINT the function returned as Python objects.
OverrideNullTypenow finds that input column by name, preferring the same position, so a function that adds or reorders columns keeps the input's types.The first two sources impose their type: the column is read into it, whatever its values.
ExecuteMapbinds each chunk once with those types declared, the pathcon.appenduses since #739 for a column whose type the caller gave, which converts each cell withTransformPythonValueand casts it into the column's type with the engine's cast. No sample decides how such a column is read, so a cell reads the same at any row. As a consequence, a cell of another kind is cast instead of refused (the text '5' in a BIGINT column given by the schema reads 5), a cell the cast refuses raises the cast's error, now naming the column (ConversionException: Conversion Error: UDF column 'a': Could not convert string 'x' to INT64), a dict without one of a STRUCT's fields reads that field as NULL, and None,duckdb.NullValue(), a NaT, pd.NA and NaN read NULL.A guess is only checked, because the function may have moved the column to another type, and casting the new values back would lose them:
df.assign(id=df.id.astype(str).str.zfill(5))over a BIGINTidwould otherwise read'00001'back as 1. Such a column is typed by the analyzer as before, and the chunk's type must equal the guess. A chunk of only missing values reads NULL, and any other type fails, naming the column andschema=. A function that hands back a typed column's values as Python objects keeps working, as before, and that one class of columns keeps the base's dependence on the analyzer's sample, which the existing strict expected failure for a fractional value outside the sample still marks. Refusing every such column instead would have changed that existing test's expectation.BindMapalso deduplicates the input names the waydf()names its columns, so both columns of one name reach the function. As indf(), the deduplication ignores case: input namesaandAreach the function asaandA_1, so a function that readsdf["A"]from such a relation now gets a KeyError. That is a visible change.On the way in,
FunctionCallturns an ENUM column into the Categoricaldf()gives it, throughEnumCategoricalDtype, whichFillNumpynow shares. The dtype is built once at bind and kept with map()'s bind data, and a returned categorical whose categories are that very index object reads its codes as the ENUM directly, through a list only map() passes toPandas::Bind. A categories index is immutable, so it still holds the enum's values, and reading them again would cost a string per category on every chunk. The check compares the index rather than the dtype, since pandas 2 builds a new dtype around the same index.con.appendand the pandas scan still read a categorical's categories as before, as does map() for a Categorical the function builds anew.A new object column that the empty bind frame cannot type stays untyped at bind. A new column of only missing values as pandas reads them (None, NaN, NaT, pd.NA) still comes back as a NULL column; a
duckdb.NullValue()is a value to pandas, so a column of them now fails like any other. One that returns values now fails on the first chunk that has them, before anything types the frame, with an error that names the column and the remedy, instead of the opaque mismatch or an error from the analyzer about a value it cannot read. The common case is a function that adds a text column on pandas 2, or on pandas 3 without pyarrow, where the empty frame gives the new column object dtype:With
schema=the function works, as before. The bind time error "Returned dataframe contains NULL type, and we could not infer the desired type", which the position match raised when the column count matched but a name did not, is gone; such a column follows the same rule as any new column.Reading every object column through the declared conversion would cost a Python value per cell, which on pandas 2, where every text column is an object column, made the identity map of text two to three times slower. The declared scan,
ScanDeclaredColumn, therefore writes a cell that is exactly a Python str, for a column declared as VARCHAR without an alias, straight into the string vector withTransformPythonString, the conversionTransformPythonValueapplies to it. Every other cell and every str subclass goes through the cast as before, and so does a VARCHAR with an alias such as JSON, whose cast validates the text. The fast path servescon.appendas well. Measured with Release builds on a shared arm64 machine at moderate load, one thread, 1M rows, the median of five per run, three runs alternating between v2.0-cyanoptera and this branch:con.appendof an object string columnThe ENUM row is the same for 3 and 1000 values; the base returns the codes rather than the ENUM. The highest values on both builds come from the noisiest of the three runs.
Merged after #8, this changes two of #8's expectations, for the one rule above: an object column whose type is imposed is read into it in one declared bind, so the analyzer no longer decides its type at execution. #8's test that an object column holding 'x' for a BIGINT column fails with "UDF column type mismatch" now expects the cast's
ConversionException. #8's caseduckdb.Value(True, duckdb.sqltypes.SQLNULL)leaves its NULL test: the declared conversion reads that value as 1, ascon.appendalready does. That a Value of type NULL should read NULL in every conversion is a separate fix.src/map.cppconflicts inExecuteMap, where both pull requests change the check of the returned types; after the merge, #8's acceptance of a returned column typed NULL only meets a column that bind left untyped. #8's other cases keep their expectations, its object column of None for a BIGINT input among them, as a guess confirmed by a chunk of only missing values.Some things stay as they are. A TIMESTAMPTZ column still reaches the function as naive UTC datetimes and comes back as TIMESTAMP; handing it over zone aware, as
df()does, would change what working functions see and waits on a decision for main. An integer or boolean column with a NULL in a chunk still arrives as float64 or object, which #8's effort handles separately, also waiting on a decision. DATE, HUGEINT and DECIMAL keepdf()'s representation (datetime64 and float64), so they come back as TIMESTAMP and DOUBLE as before. An ENUM inside a list or struct still arrives as codes, asdf()gives it. BIT, which arrives as its storage bytes, and UHUGEINT, TIMETZ and BIGNUM, which have no numpy conversion, are unchanged.The new tests pin each rule. An ENUM column is checked by dtype name against
df()at bind and per chunk, with and without a NULL, and comparing it with its text finds the rows. Every execution call sees the categories built at bind, and a Categorical built anew, ordered or not, reads as the same ENUM, or as the reordered one when its categories are reordered. The identity round trip covers seven object types with a NULL row, inferred and with an explicit schema, a STRUCT field NULL across a chunk, two columns of one name (also differing only in case), and columns added and reordered. The cast is checked both ways with the odd cell at the first row and at a row outside the analyzer's sample, as is a dict missing a STRUCT field. Further cases cover the five missing values, a zero dimensional object array, the column named in a cast error, text columns mixing str with ints and NULL, and a JSON column that still validates. A new list or text column is tested with and withoutschema=, with a cell the analyzer cannot read, and with a chunk of only None read before the chunk that fails. A typed input column moved to text, or swapped with another by a rename, is refused, and one handed back as Python ints or as None keeps its type. A new column ofduckdb.NullValue()cells, which pandas does not read as missing, is refused like any untyped column with values. 66 of the 75 fail on the merge base. The nine that pass pin unchanged behaviour: a new column of only missing values (None, NaN, pd.NA, pandas or numpy NaT) stays a NULL column, a new text column withschema=reads as before, two mixed text columns read as before, and Python ints for an INTEGER input read as before. Twocon.appendcases pin the fast path, a mixed column and a str with a lone surrogate. The existing map(), map lifetime, pandas, numpy, UDF and Spark tests pass unchanged. That includes the tests that pin an exact match for typed columns, the lifetime tests that compare map() output with a scan of the returned frame and keep a Python error's message whole, and the strict expected failure for a fractional value outside the analyzer's sample.