Skip to content

Commit 7a7876d

Browse files
authored
Get rid of unsafe logs. Fix NULL UUID case. (#3102)
* Removed unsafe logs. * Fixed uuid null handling.
1 parent 02cd19e commit 7a7876d

4 files changed

Lines changed: 101 additions & 54 deletions

File tree

cpp/deeplake_pg/duckdb_deeplake_scan.cpp

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -481,14 +481,19 @@ class deeplake_scan_function_helper
481481
auto value = base::string_view_cast<const unsigned char>(sample.data());
482482
std::string uuid_str(reinterpret_cast<const char*>(value.data()), value.size());
483483

484-
// Use DuckDB's UUID::FromString to parse UUID string
485-
try {
486-
auto uuid_value = duckdb::UUID::FromString(uuid_str);
487-
auto* duckdb_data = duckdb::FlatVector::GetData<duckdb::hugeint_t>(output_vector);
488-
duckdb_data[row_in_batch] = uuid_value;
489-
} catch (...) {
490-
// If parsing fails, set to NULL
484+
// Treat empty string as NULL for UUID columns
485+
if (uuid_str.empty()) {
491486
duckdb::FlatVector::SetNull(output_vector, row_in_batch, true);
487+
} else {
488+
// Use DuckDB's UUID::FromString to parse UUID string
489+
try {
490+
auto uuid_value = duckdb::UUID::FromString(uuid_str);
491+
auto* duckdb_data = duckdb::FlatVector::GetData<duckdb::hugeint_t>(output_vector);
492+
duckdb_data[row_in_batch] = uuid_value;
493+
} catch (...) {
494+
// If parsing fails, set to NULL
495+
duckdb::FlatVector::SetNull(output_vector, row_in_batch, true);
496+
}
492497
}
493498
}
494499
}
@@ -843,12 +848,17 @@ class deeplake_scan_function_helper
843848
auto value = td.get_streamers().value<std::string_view>(col_idx, row_idx);
844849
if (is_uuid) {
845850
std::string str_value(value);
846-
duckdb::hugeint_t uuid_value;
847-
if (!duckdb::UUID::FromString(str_value, uuid_value)) {
848-
elog(ERROR, "Failed to parse UUID string: %s", str_value.c_str());
851+
// Treat empty string as NULL for UUID columns
852+
if (str_value.empty()) {
853+
duckdb::FlatVector::SetNull(output_vector, row_in_batch, true);
854+
} else {
855+
duckdb::hugeint_t uuid_value;
856+
if (!duckdb::UUID::FromString(str_value, uuid_value)) {
857+
elog(ERROR, "Failed to parse UUID string: %s", str_value.c_str());
858+
}
859+
auto* duckdb_data = duckdb::FlatVector::GetData<duckdb::hugeint_t>(output_vector);
860+
duckdb_data[row_in_batch] = uuid_value;
849861
}
850-
auto* duckdb_data = duckdb::FlatVector::GetData<duckdb::hugeint_t>(output_vector);
851-
duckdb_data[row_in_batch] = uuid_value;
852862
} else {
853863
auto* duckdb_data = duckdb::FlatVector::GetData<duckdb::string_t>(output_vector);
854864
duckdb_data[row_in_batch] =

cpp/deeplake_pg/extension_init.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1299,9 +1299,9 @@ static void executor_run(QueryDesc* query_desc, ScanDirection direction, uint64
12991299
pg::query_info::cleanup();
13001300
pg::table_storage::instance().reset_requested_columns();
13011301
} catch (const std::exception& e) {
1302-
elog(WARNING, "Error during transaction cleanup: %s", e.what());
1302+
// Silently handle cleanup errors
13031303
} catch (...) {
1304-
elog(WARNING, "Unknown error during transaction cleanup");
1304+
// Silently handle cleanup errors
13051305
}
13061306
PG_RE_THROW();
13071307
}
@@ -1332,7 +1332,6 @@ static void executor_end(QueryDesc* query_desc)
13321332
// Error occurred during flush - rollback and suppress to prevent cascade
13331333
// This prevents "Deeplake does not support transaction aborts" cascade
13341334
pg::table_storage::instance().rollback_all();
1335-
elog(WARNING, "Failed to flush data during executor end, changes rolled back");
13361335
// Don't re-throw - let the transaction abort naturally
13371336
FlushErrorState();
13381337
}

cpp/deeplake_pg/reporter.hpp

Lines changed: 1 addition & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
#pragma once
22

3-
#include <base/backtrace.hpp>
4-
#include <base/memory_info.hpp>
5-
#include <base/system_report.hpp>
6-
73
#include <chrono>
84
#include <csignal>
95
#include <string>
@@ -12,27 +8,8 @@ namespace pg {
128

139
extern bool print_runtime_stats;
1410

15-
inline static void print_memory_report()
16-
{
17-
base::memory_info info;
18-
19-
base::system_report::get_meminfo(info);
20-
const auto msg = fmt::format("Memory Report:\n"
21-
"RSS: ({:.2f} MB)"
22-
", VM: ({:.2f} MB)"
23-
", Peak: ({:.2f} MB)",
24-
info.process_vm_rss / (1024.0 * 1024.0),
25-
info.process_vm_size / (1024.0 * 1024.0),
26-
info.process_peak_mem / (1024.0 * 1024.0));
27-
elog(INFO, "%s", msg.c_str());
28-
}
29-
3011
inline static void signal_handler(int signal)
3112
{
32-
elog(NOTICE, "Caught signal %d (%s)\n", signal, strsignal(signal));
33-
elog(NOTICE, "%s", base::backtrace().c_str());
34-
print_memory_report();
35-
fflush(stderr);
3613
_exit(signal);
3714
}
3815

@@ -61,22 +38,7 @@ struct runtime_printer
6138
}
6239
}
6340

64-
~runtime_printer()
65-
{
66-
if (pg::print_runtime_stats) {
67-
auto end_time = std::chrono::high_resolution_clock::now();
68-
auto duration = std::chrono::duration<double, PERIOD>(end_time - start_time_).count();
69-
std::string period_name = "ms";
70-
if constexpr (std::is_same_v<PERIOD, std::micro>) {
71-
period_name = "µs";
72-
} else if constexpr (std::is_same_v<PERIOD, std::nano>) {
73-
period_name = "ns";
74-
} else if constexpr (std::is_same_v<PERIOD, std::ratio<1>>) {
75-
period_name = "s";
76-
}
77-
elog(INFO, "%s: %.2f %s", task_name_.data(), duration, period_name.data());
78-
}
79-
}
41+
~runtime_printer() = default;
8042

8143
private:
8244
std::string_view task_name_;

postgres/tests/py_tests/test_uuid.py

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,79 @@ async def test_uuid_type(db_conn: asyncpg.Connection):
7979
finally:
8080
# Cleanup
8181
await db_conn.execute("DROP TABLE IF EXISTS people CASCADE")
82+
83+
84+
@pytest.mark.asyncio
85+
async def test_uuid_empty_string_handling(db_conn: asyncpg.Connection):
86+
"""
87+
Test that empty strings in UUID columns are handled correctly.
88+
89+
This test verifies the fix for the issue where adding a UUID column
90+
to a table with existing rows would cause "Failed to parse UUID string:"
91+
error when querying. Empty strings stored in deeplake are treated as NULL.
92+
93+
Tests the exact scenario:
94+
1. Create table without UUID column
95+
2. Add UUID column (existing rows get NULL/empty values)
96+
3. Query table (should not crash)
97+
4. Insert row with NULL UUID
98+
5. Query multiple times
99+
"""
100+
assertions = Assertions(db_conn)
101+
102+
try:
103+
# Request 1: Create table uuid_test
104+
await db_conn.execute("""
105+
CREATE TABLE uuid_test (
106+
id SERIAL PRIMARY KEY,
107+
name TEXT NOT NULL
108+
) USING deeplake
109+
""")
110+
111+
# Request 5: Query on uuid_test table
112+
rows = await db_conn.fetch(
113+
"SELECT * FROM (SELECT * FROM uuid_test ORDER BY id) LIMIT 20 OFFSET 0"
114+
)
115+
assert len(rows) == 0, f"Expected 0 rows, got {len(rows)}"
116+
117+
# Request 6: Add UUID column to uuid_test
118+
await db_conn.execute("""
119+
ALTER TABLE uuid_test ADD COLUMN uu UUID
120+
""")
121+
122+
# Request 8: Query uuid_test after schema change
123+
# This would previously crash with: ERROR: Failed to parse UUID string:
124+
rows = await db_conn.fetch(
125+
"SELECT * FROM (SELECT * FROM uuid_test ORDER BY id) LIMIT 20 OFFSET 0"
126+
)
127+
assert len(rows) == 0, f"Expected 0 rows after adding UUID column, got {len(rows)}"
128+
129+
# Request 9: Insert row with empty name and NULL UUID
130+
await db_conn.execute("""
131+
INSERT INTO uuid_test (name, uu) VALUES ('', NULL)
132+
""")
133+
134+
# Request 11: Query uuid_test after insert
135+
rows = await db_conn.fetch(
136+
"SELECT * FROM (SELECT * FROM uuid_test ORDER BY id) LIMIT 20 OFFSET 0"
137+
)
138+
assert len(rows) == 1, f"Expected 1 row after insert, got {len(rows)}"
139+
assert rows[0]['uu'] is None, "UUID should be NULL"
140+
141+
# Request 12: Query uuid_test again
142+
rows = await db_conn.fetch(
143+
"SELECT * FROM (SELECT * FROM uuid_test ORDER BY id) LIMIT 20 OFFSET 0"
144+
)
145+
assert len(rows) == 1, f"Expected 1 row, got {len(rows)}"
146+
147+
# Request 13: Query uuid_test again
148+
rows = await db_conn.fetch(
149+
"SELECT * FROM (SELECT * FROM uuid_test ORDER BY id) LIMIT 20 OFFSET 0"
150+
)
151+
assert len(rows) == 1, f"Expected 1 row, got {len(rows)}"
152+
153+
print("✓ Test passed: UUID empty string handling works correctly")
154+
155+
finally:
156+
# Cleanup
157+
await db_conn.execute("DROP TABLE IF EXISTS uuid_test CASCADE")

0 commit comments

Comments
 (0)