introduce ndarr support #98

kafka1991 · 2025-04-01T15:04:27Z

introduce ndarr support

amunra · 2025-04-03T16:55:14Z

questdb-rs/src/ingress/mod.rs

+        let index = self.output.len();
+        let reserve_size = reserve_size.checked_add(index).ok_or(error::fmt!(
+            ArrayViewError,
+            "Array total elem size overflow"
+        ))?;
+        self.output
+            .try_reserve(reserve_size)
+            .map_err(|_| error::fmt!(BufferOutOfMemory, "Buffer out of memory"))?;


That's not how reserve works: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.reserve

You pass in the argument for additional required size, not the total.

Also try_reserve is completely pointless here since we can end up with large memory pressure already: A regular reserve should do.

amunra · 2025-04-03T17:00:37Z

questdb-rs/src/ingress/mod.rs

+        unsafe { self.output.set_len(reserve_size) }
+
+        // ndarr data
+        view.write_row_major_buf(&mut self.output[index..]);


Security!
The memory would most likely not be initialized, I'll need to think about this in more depth as it could be an attack vector in the application.

Ok.. there's certainly no attack vector that I can think of, but it is easy for an API to write too little data and the we'd end up with junk data in the db.

This bit I think we ought to do better.

I suggest:

// NB: I haven't run any of this, my code is probably rather broken :-) let output_len = self.output.len(); let writable = unsafe { std::slice::from_raw_parts_mut( self.output.ptr_mut().add(output_len), array_buf_size) }; let mut cursor = Cursor::new(writable); if let Err(..) = view.write_row_major(&mut cursor) { ... add context to the error and return `Err` ... } if cursor.position() != (array_buf_size as u64) { return Err(... tell the user that their array write op was the wrong size ...); } // Update last so in case of errors we haven't grown the buffer. unsafe { self.output.set_len(output_len + array_buf_size) };

It should be logically very similar, one extra check and if I'm not wrong (I could be) the cursor should not perform an extra copy, but this last detail should probably be triple-checked by reading the standard lib impl.

I forgot to mention.. the trait method would then have a different signature:

fn write_row_major<W: std::io::Write>(&self, dest: &mut W) -> std::io::Result<()>;

amunra · 2025-04-03T17:02:47Z

questdb-rs/src/ingress/ndarr.rs

@@ -0,0 +1,133 @@
+pub(crate) const MAX_DIMS: usize = 32;


I'd move this top-level in questb::ingress and make it public, then add a doc to it. It might be useful to callers.

amunra · 2025-04-04T12:20:16Z

questdb-rs/src/error.rs

+    /// Buffer outOfMemory.
+    BufferOutOfMemory,


amunra · 2025-04-04T12:26:05Z

questdb-rs/src/ingress/ndarr.rs

+
+#[repr(u8)]
+#[derive(Debug, PartialEq)]
+pub enum ElemDataType {


Where does this come from?
Is there a spec document that maps the supported datatypes?

If there's a format document link to it, otherwise let's write a spec for it and link to it.
This enum here is just too confusing and too error-prone otherwise.

Each entry should also be explicit about its value, e.g. Boolean = 1, Byte = 2 .. etc. This is to quickly be able to cross-reference a format spec and avoid the enum getting accidentally reordered and breaking everything.

Additionally, why is there an Undefined entry?

PS, do we really intend to support all of these datatypes as binary?

The current version only supports double. We have plans to support more data types in the future.

Then this list should only contain double :-)

amunra · 2025-04-04T12:30:23Z

questdb-rs/src/ingress/mod.rs

@@ -24,8 +24,8 @@

 #![doc = include_str!("mod.md")]

+pub use self::ndarr::*;


The * risks leaking too many types.
Instead just pick the ones you explicitly are sure you want in the public API.

amunra · 2025-04-04T12:35:04Z

questdb-rs/src/tests/mock.rs

+        self.msgs = accum;
+        Ok(self.msgs.len())


?

The previous code was there to handle cases where multiple messages are received in a single chunk. I don't think that possibility goes away.

In binary format, the binary data might inherently contain characters that would normally separate rows (\n). Distinguishing rows in this case can be achieved through further parsing of the binary data. However, implementing this is relatively complex and would require supporting binary format parsing in mock.rs. Therefore, for convenience, the output in the test scenario has been changed.

Yeah, but ignoring the problem is not a solution :-)

This makes the tests frail and prone to occasionally fail.

Why would this make the tests more frail? Compared to the previous testing method, this only changes the returned number of msgs to the returned number of bytes. If network uncertainties cause the number of received bytes to be uncertain here, that issue existed previously. IMHO, this doesn't makes the problem any worse.

amunra · 2025-04-04T12:37:59Z

questdb-rs/src/tests/mod.rs

@@ -29,6 +29,9 @@ mod http;
 mod mock;
 mod sender;

+#[cfg(feature = "ndarray")]
+mod ndarr;


There should be tests for the trait directly as well, even without ndarray enabled.

This is to be sure that we never tie the ndarray feature to the ability to write arrays.

The tests that check the NdArrayView trait should also check what happens when the trait does nothing or less data.

Another test to write is if the trait gives back the wrong number of dimensions.

amunra · 2025-04-04T12:44:26Z

include/questdb/ingress/line_sender.hpp

+    template <typename T, size_t N>
+    line_sender_buffer& column(
+        column_name_view name,
+        const std::vector<uint32_t>& shape,
+        const std::array<T, N>& data)


We need to come up with something that's a bit more idiomatic C++.
Let's park the C++ API for now until we have everything else nailed down.

amunra · 2025-04-04T15:42:06Z

questdb-rs/src/ingress/ndarr.rs

+        }
+    }
+
+    fn write_row_major_buf(&self, buf: &mut [u8]) {


The implementation here is incomplete.

It should handle cases where the strides are non-row-major standard.
The current code will serialize the same buffer for both array A and its transpose A'.

Worth having a bunch of independent unit tests (without constructing full buffers) that just test this single trait method for different ndarrays of different shapes and strides.

The implementation here is incomplete.

It should handle cases where the strides are non-row-major standard.
The current code will serialize the same buffer for both array A and its transpose A'.

These cases are handled here, with corresponding tests (test_2d_non_contiguous_buffer and test_strided_layout). When the array is non-row-major, view.as_slice() returns None, and the array's iterator(have considerd all kinds of layouts inner) is used to copy individual elements.
Or did I miss something?

# Conflicts: # questdb-rs/Cargo.toml

c client nd array begin.

87aa240

kafka1991 self-assigned this Apr 1, 2025

kafka1991 marked this pull request as draft April 1, 2025 15:04

kafka1991 added 10 commits April 2, 2025 19:26

Ndarray support for rust client.

6bd3223

optimize doc.

b518a99

add tests.

53a430e

fix comments.

b1ebd1d

fix docs.

006af98

fix tests and compile error in ffi.

a31716a

format code.

463a2d3

ffi supported

44fae08

ndarray c api

93c6f9d

format

e9da558

kafka1991 marked this pull request as ready for review April 3, 2025 09:05

kafka1991 added 3 commits April 3, 2025 21:36

c++ api support.

0e8a9e5

add include.

261163c

code format.

a565109

amunra reviewed Apr 3, 2025

View reviewed changes

amunra reviewed Apr 4, 2025

View reviewed changes

kafka1991 added 3 commits April 6, 2025 22:21

Merge remote-tracking branch 'origin/main' into array

d1edbbc

# Conflicts: # questdb-rs/Cargo.toml

conflict resolve

e161b1d

code review.

4a59ec0

kafka1991 and others added 20 commits April 9, 2025 16:22

code format

feaf5be

make c api works.

c4bed10

make c api works.

f3011e1

code format

70eaa24

make c++ tests work.

526e8ae

fix compile error.

9c16d74

make c++20 build happy

5dae9d0

code format

ea52293

fix cpp problem on windows.

193b887

lint issues

523a855

add python tests.

a1fb92d

add stridesArrayView performance benchmark.

7b75fc6

introduce double binary format protocol.

4b5655a

code format

4076336

fix little issue

612406f

f64 binary format test.

e1ed453

f64 binary format test.

888665d

fix cpp test for double binary format.

1b3e755

fix tests

ceac17d

use direct buffer case when ndarray is not continus.

c38c083

kafka1991 requested a review from amunra April 23, 2025 11:46

kafka1991 and others added 9 commits April 23, 2025 19:56

ci import numpy

21cd6f9

temp fix ci to specify nd_arr branch

1f17a09

temp fix ci to specify nd_arr branch

73f784a

temp fix ci tests

5de599e

fix ci tests

37f8d20

fix tests

32f2a63

add cpp array example.

1f12f27

fix logs

89eaa79

fix array tests

97c3179

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce ndarr support #98

introduce ndarr support #98

kafka1991 commented Apr 1, 2025

amunra Apr 3, 2025

kafka1991 Apr 7, 2025

amunra Apr 3, 2025 •

edited

Loading

amunra Apr 4, 2025 •

edited

Loading

amunra Apr 4, 2025

kafka1991 Apr 7, 2025

amunra Apr 3, 2025

kafka1991 Apr 7, 2025

amunra Apr 4, 2025

kafka1991 Apr 7, 2025

amunra Apr 4, 2025 •

edited

Loading

amunra Apr 4, 2025

kafka1991 Apr 4, 2025

amunra Apr 4, 2025

kafka1991 Apr 7, 2025

amunra Apr 4, 2025

amunra Apr 4, 2025

kafka1991 Apr 4, 2025

amunra Apr 4, 2025

kafka1991 Apr 6, 2025 •

edited

Loading

amunra Apr 4, 2025

amunra Apr 4, 2025

amunra Apr 4, 2025

amunra Apr 4, 2025

amunra Apr 4, 2025 •

edited

Loading

kafka1991 Apr 6, 2025 •

edited

Loading

		@@ -24,8 +24,8 @@

		#![doc = include_str!("mod.md")]

		pub use self::ndarr::*;

introduce ndarr support #98

Are you sure you want to change the base?

introduce ndarr support #98

Conversation

kafka1991 commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amunra Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

amunra Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amunra Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kafka1991 Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amunra Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

kafka1991 Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

amunra Apr 3, 2025 •

edited

Loading

amunra Apr 4, 2025 •

edited

Loading

amunra Apr 4, 2025 •

edited

Loading

kafka1991 Apr 6, 2025 •

edited

Loading

amunra Apr 4, 2025 •

edited

Loading

kafka1991 Apr 6, 2025 •

edited

Loading