Switch to serde and clean warnings #83

erg · 2025-11-15T01:00:32Z

update to serde. codex did this

vmx

First of all: are you kidding me? Today I thought, tonight I'll spend some time on Noise. I planned to add a Rust based HTTP API (instead of the current Node.js based one which no longer works properly), but now I did this review.

Second: thanks for the PR!

Generally I prefer separating the concerns in individual PRs/commits. But given I haven't spent much time on this project recently, I can see that also fixing things at the same time makes sense.

I really like the refactoring around the geo stuff. Having it as a separate struct is so much cleaner.

Switching to serde_json makes sense, though when I thought about getting rid of the rustc_serialize is thought about using some JSON parser that is streaming and more bare-bones as we mostly need kind of the tokens. As I never head the time to look into it, I'm happy to use serde_json for now.

I don't get the src/bin additions. It looks like they were from some debugging session. I would rather see them as proper tests and not as executables.

I've some inline comments, but they are mostly minor ones.

vmx · 2025-11-15T20:54:13Z

src/filters.rs

+        // For array queries, we need to consider the context
+        let min_seq = if self.kb.arraypath.is_empty() {
+            // Not an array query, use normal start seq
+            start.seq
+        } else if start.seq > 0 && start.arraypath.len() == self.kb.arraypath.len() {
+            // Array query for a specific document - search within that document
+            // by using seq-1 to ensure we get all array elements
+            start.seq.saturating_sub(1)
+        } else {
+            start.seq
+        };


Having two conditional code paths with the same value feels strange. It's about whether it's an array query or not. So I'd rather do something like (not tested):

Suggested change

// For array queries, we need to consider the context

let min_seq = if self.kb.arraypath.is_empty() {

// Not an array query, use normal start seq

start.seq

} else if start.seq > 0 && start.arraypath.len() == self.kb.arraypath.len() {

// Array query for a specific document - search within that document

// by using seq-1 to ensure we get all array elements

start.seq.saturating_sub(1)

} else {

start.seq

};

// For array queries, we need to consider the context

let min_seq = if self.kb.arraypath > 0 && start.seq > 0 && start.arraypath.len() && self.kb.arraypath.len()` {

// Array query for a specific document - search within that document

// by using seq-1 to ensure we get all array elements

start.seq.saturating_sub(1)

} else {

start.seq

};

vmx · 2025-11-15T21:10:43Z

src/filters.rs

+                // Get arraypath for this result
+                let arraypath = BboxFilter::from_u8_slice(&value);
+
+                // Skip if we've already returned this exact result


It's been a very long time since I've been deep in the Noise code base. So it's a bit as if I read it with a fresh eye. Could you please add a comment when/why this case is happening?

vmx · 2025-11-15T21:14:54Z

src/index.rs

+            if bytes.is_empty() {
+                0
+            } else {


The Index::convert_bytes_to_i32() already contains the bytes.is_empty() check. So this check can be removed.

vmx · 2025-11-15T21:15:50Z

src/index.rs


        for bytes in operands {
-            count += Index::convert_bytes_to_i32(bytes);
+            if !bytes.is_empty() {


Same here as in the comment above. The Index::convert_bytes_to_i32() would just add 0 to the count. So this if can be removed.

vmx · 2025-11-15T22:06:54Z

src/key_builder.rs

+    /// Get the keypath as a concatenated string
+    pub fn keypath_string(&self) -> String {
+        let mut keypath = String::with_capacity(100);
+        for segment in &self.keypath {
+            keypath.push_str(segment);
+        }
+        keypath
+    }


This doesn't seem to be used anywhere. If it is used somewhere (or planned), I'd change it to:

Suggested change

/// Get the keypath as a concatenated string

pub fn keypath_string(&self) -> String {

let mut keypath = String::with_capacity(100);

for segment in &self.keypath {

keypath.push_str(segment);

}

keypath

}

/// Get the keypath as a concatenated string

pub fn keypath_string(&self) -> String {

self.keypath.into_iter().collect::<String>()

}

vmx · 2025-11-15T22:08:10Z

src/lib.rs

 mod returnable;
 mod snapshot;
 mod stems;
+mod test_geometry;


That module seems to be missing in this PR.

vmx · 2025-11-15T22:13:28Z

.cargo/config

+[build]
+rustflags = ["-C", "link-arg=-undefined", "-C", "link-arg=dynamic_lookup"]
+
+[env]
+CXXFLAGS = "-std=c++11 -Wno-unused-parameter"


It doesn't compile for me with those settings. I'm on Linux (Debian testing). So maybe those shouldn't be checked in?

vmx · 2025-11-15T22:32:22Z

src/query.rs

                "query cannot be made up of only logical not. Must have at \
                                     least one match clause not negated."
-                    .to_string(),
+                    .to_owned(),


All those to_string() -> to_owned() don't really matter. I wouldn't make that change, but it also isn't a blocker.

erg · 2025-11-23T23:33:11Z

Hey, I have been running some LLMs in the background -- they can finally do real, complicated stuff like this! (purely letting codex do this so far)

I didn't see your comments til now though, and honestly, I didn't look at the code at all! It's obviously terrible not to look at the code, but I was trying to gauge interest.

I'll try to address your concerns and clean it up!

vmx · 2025-11-24T11:31:08Z

I suspected that. I though was surely surprised that it was that good. So next time it would be good if you could add expectations to reviewers. Something like "rough idea and first cut, are we interested in a cleaned up version of this" or "this is ready for an in depth review" or of course anything in between.

I again want to spend more time on Noise. I've a few things in mind I want to tackle next. Maybe I should open issues, so that you can do some experimenting with LLMs if you like.

Please also have a look at the rust-rocksdb PR it created. After a quick look it doesn't really seem to be needed at all.

erg · 2025-11-24T18:04:01Z

I thought "codex did this" was sufficient to warn that I don't endorse this code, only that it's possible to ditch the unmaintained serialization library and that the ai has caught up to be able to work on this project and be useful.

It's also been a few years since the last commit, and Damien is mia. I'm fine with being something more professional in the future, didn't mean to do anything other than nudge the project a bit.

Switch to serde and clean warnings

c496501

erg mentioned this pull request Nov 15, 2025

Update to serde pipedown/rust-rocksdb#3

Open

vmx requested changes Nov 15, 2025

View reviewed changes

Switch to serde and clean warnings #83

Are you sure you want to change the base?

Switch to serde and clean warnings #83

Uh oh!

Conversation

erg commented Nov 15, 2025

Uh oh!

vmx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erg commented Nov 23, 2025

Uh oh!

vmx commented Nov 24, 2025

Uh oh!

erg commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants