Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java] BigDecimal scale > precision bug fix #6110

Merged
merged 2 commits into from
Feb 12, 2023
Merged

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Feb 6, 2023

This PR fixes #6073

As the title says, in java BigDecimal can have a scale that is larger than its precision.
Since we map directly precision -> width this causes a problem internally because we assert that width >= scale.

@Tishj
Copy link
Contributor Author

Tishj commented Feb 6, 2023

I am running into an issue here now, where we run into an Allocator assertion, because something is not being cleaned up properly
The trace says it originates from execute()

Outstanding allocations found for Allocator
Allocation of size 32768 at address 0x101855c00
Stack trace:

0   libduckdb_java365074006139430068.so 0x0000000173691ff0 _ZN6duckdb9Exception13GetStackTraceEi + 100
1   libduckdb_java365074006139430068.so 0x00000001736913d0 _ZN6duckdb18AllocatorDebugInfo12AllocateDataEPhy + 68
2   libduckdb_java365074006139430068.so 0x00000001736911dc _ZN6duckdb9Allocator12AllocateDataEy + 420
3   libduckdb_java365074006139430068.so 0x0000000173448684 _ZN6duckdb9Allocator8AllocateEy + 44
4   libduckdb_java365074006139430068.so 0x0000000173448534 _ZN6duckdb19ColumnDataAllocator14AllocateMemoryEyRjS1_PNS_20ChunkManagementStateE + 224
5   libduckdb_java365074006139430068.so 0x000000017344886c _ZN6duckdb19ColumnDataAllocator12AllocateDataEyRjS1_PNS_20ChunkManagementStateE + 264
6   libduckdb_java365074006139430068.so 0x0000000173450ba8 _ZN6duckdb27ColumnDataCollectionSegment22AllocateVectorInternalERKNS_11LogicalTypeERNS_13ChunkMetaDataEPNS_20ChunkManagementStateE + 192
7   libduckdb_java365074006139430068.so 0x0000000173450e14 _ZN6duckdb27ColumnDataCollectionSegment14AllocateVectorERKNS_11LogicalTypeERNS_13ChunkMetaDataEPNS_20ChunkManagementStateENS_15VectorDataIndexE + 64
8   libduckdb_java365074006139430068.so 0x000000017344c364 _ZN6duckdb27ColumnDataCollectionSegment16AllocateNewChunkEv + 172
9   libduckdb_java365074006139430068.so 0x000000017344107c _ZN6duckdb20ColumnDataCollection16InitializeAppendERNS_21ColumnDataAppendStateE + 180
10  libduckdb_java365074006139430068.so 0x0000000173c9f600 _ZN6duckdb13ClientContext19FetchResultInternalERNS_17ClientContextLockERNS_18PendingQueryResultE + 1012
11  libduckdb_java365074006139430068.so 0x0000000173ca5470 _ZN6duckdb18PendingQueryResult15ExecuteInternalERNS_17ClientContextLockE + 168
12  libduckdb_java365074006139430068.so 0x0000000173ca795c _ZN6duckdb18PendingQueryResult7ExecuteEv + 76
13  libduckdb_java365074006139430068.so 0x0000000173caf38c _ZN6duckdb17PreparedStatement7ExecuteERNSt3__16vectorINS_5ValueENS1_9allocatorIS3_EEEEb + 212
14  libduckdb_java365074006139430068.so 0x000000017247e938 Java_org_duckdb_DuckDBNative_duckdb_1jdbc_1execute + 2776
15  ???                                 0x0000000117af18ac 0x0 + 4692318380
16  ???                                 0x0000000117aedd80 0x0 + 4692303232
17  ???                                 0x0000000117aee3a8 0x0 + 4692304808
18  ???                                 0x0000000117aedfc8 0x0 + 4692303816
19  ???                                 0x0000000117aedfc8 0x0 + 4692303816
20  ???                                 0x0000000117aedd80 0x0 + 4692303232
21  ???                                 0x0000000117aedd80 0x0 + 4692303232
22  ???                                 0x0000000117aedd80 0x0 + 4692303232
23  ???                                 0x0000000117aee2c0 0x0 + 4692304576
24  ???                                 0x0000000117aedd80 0x0 + 4692303232
25  ???                                 0x0000000117ae8140 0x0 + 4692279616
26  libjvm.dylib                        0x0000000102a3b7d8 _ZN9JavaCalls11call_helperEP9JavaValueRK12methodHandleP17JavaCallArgumentsP10JavaThread + 564
27  libjvm.dylib                        0x0000000102a938a4 _ZL17jni_invoke_staticP7JNIEnv_P9JavaValueP8_jobject11JNICallTypeP10_jmethodIDP18JNI_ArgumentPusherP10JavaThread + 204
28  libjvm.dylib                        0x0000000102a96574 jni_CallStaticVoidMethod + 216
29  libjli.dylib                        0x00000001000bd66c JavaMain + 2856
30  libjli.dylib                        0x00000001000bf7cc ThreadJavaMain + 12
31  libsystem_pthread.dylib             0x000000019639206c _pthread_start + 148
32  libsystem_pthread.dylib             0x000000019638ce2c thread_start + 8

I have traced it down to either one of the PreparedStatements used in the test_lots_of_decimals loop.
As the trace says execute and not executeQuery I am tempted to say it's the insert, not the select.

But after an hour I am giving up on debugging this for now, I'm getting nowhere still

@Jens-H
Copy link
Contributor

Jens-H commented Feb 7, 2023

I had a look at the Java_org_duckdb_DuckDBNative_duckdb_1jdbc_1execute function and the return value caught my attention:
return env->NewDirectByteBuffer(res_ref.release(), 0);

This is freed with Java_org_duckdb_DuckDBNative_duckdb_1jdbc_1free_1result, but execute() is not calling it.

@Tishj
Copy link
Contributor Author

Tishj commented Feb 8, 2023

I see what you mean, I thought the boolean value would indicate if freeing is necessary, looks like it's not.

I think it's the responsibility of the caller to free the result of execute, I'll check if the calls to free fix the issue.

There are other examples similar to this test that do behave properly though

@Jens-H
Copy link
Contributor

Jens-H commented Feb 8, 2023

I think I forgot to call free at least here:

And here it might be missing also, if multiple calls of execute happen:


@Tishj
Copy link
Contributor Author

Tishj commented Feb 9, 2023

➜  duckdb git:(java_decimal) ✗ java -cp build/debug/tools/jdbc/duckdb_jdbc.jar org.duckdb.test.TestDuckDBJDBC test_lots_of_decimals
test_lots_of_decimals success in 60 seconds
OK

👍

Thanks for your help, that was exactly it.
That's the pain of wrapping a C API, you inherit the manual memory management haha

@Tishj
Copy link
Contributor Author

Tishj commented Feb 12, 2023

@Mytherin could you re-run the python test?

@Mytherin Mytherin merged commit 5eb6aa9 into duckdb:master Feb 12, 2023
@Mytherin
Copy link
Collaborator

I will just merge it instead - I believe that changing the JDBC code has not broken the Python client :)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestDuckDBJDBC::test_lots_of_decimals is broken
3 participants