Skip to content

Commit 03cbf14

Browse files
committed
RDBC-1068 Fix C#-contradiction bugs found in post-sync audit
- ChunkingOptions.from_json: default MaxTokensPerChunk/OverlapTokens to 512/0 (matching C#'s non-nullable int initializers) instead of None, which crashed validate() with a TypeError on missing keys. - EmbeddingsGenerationConfiguration.validate: drop the spurious 'cannot specify both paths and transformation' rejection (C# allows both), and add the per-path ChunkingOptions validation loop C# performs. - OrderByToken.create_distance_ascending_wkt: emit spatial.distance(field, spatial.wkt(...)) instead of the malformed spatial.distance(field), spatial.wkt(...) (stray paren). Each fix has a regression test that fails on the pre-fix code.
1 parent f7c9cc2 commit 03cbf14

6 files changed

Lines changed: 124 additions & 5 deletions

File tree

ravendb/documents/operations/ai/chunking_options.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ def __init__(
4444
def from_json(cls, json_dict: Dict[str, Any]) -> "ChunkingOptions":
4545
return cls(
4646
chunking_method=ChunkingMethod(json_dict["ChunkingMethod"]),
47-
max_tokens_per_chunk=json_dict.get("MaxTokensPerChunk", None),
48-
overlap_tokens=json_dict.get("OverlapTokens", None),
47+
max_tokens_per_chunk=json_dict.get("MaxTokensPerChunk", 512),
48+
overlap_tokens=json_dict.get("OverlapTokens", 0),
4949
context_prefix=json_dict.get("ContextPrefix", None),
5050
)
5151

ravendb/documents/operations/ai/embeddings_generation_configuration.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,8 +135,14 @@ def validate(
135135

136136
if not has_paths and not has_transformation:
137137
errors.append("Either EmbeddingsPathConfigurations or EmbeddingsTransformation must be provided")
138-
elif has_paths and has_transformation:
139-
errors.append("Cannot specify both EmbeddingsPathConfigurations and EmbeddingsTransformation")
138+
139+
# Validate each path's chunking options (mirrors the server / C# client; both may be set).
140+
if self.embeddings_path_configurations:
141+
for path_configuration in self.embeddings_path_configurations:
142+
if path_configuration.chunking_options is not None:
143+
path_configuration.chunking_options.validate(path_configuration.path, errors)
144+
else:
145+
errors.append(f"Path '{path_configuration.path}': ChunkingOptions must be provided.")
140146

141147
# Validate transformation if provided
142148
if has_transformation:

ravendb/documents/session/tokens/query_tokens/definitions.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ def create_distance_ascending_wkt(
299299
nulls: NullsOrdering = NullsOrdering.DEFAULT,
300300
) -> OrderByToken:
301301
return cls(
302-
f"spatial.distance({field_name}), "
302+
f"spatial.distance({field_name}, "
303303
f"spatial.wkt(${wkt_parameter_name})"
304304
f"{'' if round_factor_parameter_name is None else ', $' + round_factor_parameter_name})",
305305
False,

ravendb/tests/embeddings_generation_tests/test_chunking_options.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,17 @@ def test_overlap_rejected_on_non_paragraph_methods(self):
6464
self.assertEqual(1, len(errors), method)
6565
self.assertIn("OverlapTokens", errors[0])
6666

67+
def test_from_json_missing_int_keys_use_csharp_defaults(self):
68+
# Missing MaxTokensPerChunk/OverlapTokens must default to 512/0 (matching C#'s non-nullable
69+
# int initializers), not None. None would crash validate() with a TypeError.
70+
options = ChunkingOptions.from_json({"ChunkingMethod": "HtmlStrip"})
71+
self.assertEqual(512, options.max_tokens_per_chunk)
72+
self.assertEqual(0, options.overlap_tokens)
73+
74+
errors = []
75+
options.validate("source", errors) # must not raise TypeError on the defaults
76+
self.assertEqual([], errors)
77+
6778

6879
if __name__ == "__main__":
6980
unittest.main()
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import unittest
2+
3+
from ravendb.documents.operations.ai.chunking_options import ChunkingOptions, ChunkingMethod
4+
from ravendb.documents.operations.ai.embedding_path_configuration import EmbeddingPathConfiguration
5+
from ravendb.documents.operations.ai.embeddings_generation_configuration import EmbeddingsGenerationConfiguration
6+
from ravendb.documents.operations.ai.embeddings_transformation import EmbeddingsTransformation
7+
8+
9+
def _config(**overrides) -> EmbeddingsGenerationConfiguration:
10+
# A base config that passes every check except the paths/transformation logic under test.
11+
base = dict(
12+
name="emb",
13+
identifier="emb",
14+
collection="Docs",
15+
connection_string_name="cs",
16+
chunking_options_for_querying=ChunkingOptions(ChunkingMethod.HTML_STRIP, 100, 0),
17+
)
18+
base.update(overrides)
19+
return EmbeddingsGenerationConfiguration(**base)
20+
21+
22+
def _validate(config) -> list:
23+
return config.validate(validate_name=False, validate_identifier=False)
24+
25+
26+
class TestEmbeddingsGenerationConfigurationValidate(unittest.TestCase):
27+
def test_paths_and_transformation_together_are_allowed(self):
28+
# C# has no mutual-exclusivity rule; setting both must NOT be rejected client-side.
29+
config = _config(
30+
embeddings_path_configurations=[
31+
EmbeddingPathConfiguration("Name", ChunkingOptions(ChunkingMethod.HTML_STRIP, 100, 0))
32+
],
33+
embeddings_transformation=EmbeddingsTransformation(script="embeddings.generate(this.Name)"),
34+
)
35+
errors = _validate(config)
36+
self.assertNotIn("Cannot specify both EmbeddingsPathConfigurations and EmbeddingsTransformation", errors)
37+
self.assertEqual([], errors)
38+
39+
def test_path_missing_chunking_options_is_rejected(self):
40+
# Mirrors C#: each path must carry ChunkingOptions.
41+
config = _config(embeddings_path_configurations=[EmbeddingPathConfiguration("Name", None)])
42+
errors = _validate(config)
43+
self.assertIn("Path 'Name': ChunkingOptions must be provided.", errors)
44+
45+
def test_path_invalid_chunking_options_is_rejected(self):
46+
# Each path's ChunkingOptions is validated with the path as the error source.
47+
config = _config(
48+
embeddings_path_configurations=[
49+
EmbeddingPathConfiguration("Name", ChunkingOptions(ChunkingMethod.HTML_STRIP, 0, 0))
50+
]
51+
)
52+
errors = _validate(config)
53+
self.assertTrue(
54+
any("Name" in e and "MaxTokensPerChunk" in e for e in errors),
55+
f"expected a per-path MaxTokensPerChunk error, got: {errors}",
56+
)
57+
58+
def test_path_with_valid_chunking_options_passes(self):
59+
config = _config(
60+
embeddings_path_configurations=[
61+
EmbeddingPathConfiguration("Name", ChunkingOptions(ChunkingMethod.HTML_STRIP, 100, 0))
62+
]
63+
)
64+
self.assertEqual([], _validate(config))
65+
66+
67+
if __name__ == "__main__":
68+
unittest.main()
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import unittest
2+
3+
from ravendb.documents.session.tokens.query_tokens.definitions import OrderByToken
4+
5+
6+
def _rql(token: OrderByToken) -> str:
7+
writer = []
8+
token.write_to(writer)
9+
return "".join(writer)
10+
11+
12+
class TestOrderByDistanceWktRql(unittest.TestCase):
13+
"""The WKT ascending distance factory must nest spatial.wkt(...) inside spatial.distance(field, ...),
14+
matching C# (OrderByToken.cs) and its three sibling factories. The bug emitted a stray ')' right
15+
after the field name: 'spatial.distance(loc), spatial.wkt(...)'."""
16+
17+
def test_distance_ascending_wkt_nests_wkt_inside_distance(self):
18+
rql = _rql(OrderByToken.create_distance_ascending_wkt("loc", "p0", None))
19+
self.assertIn("spatial.distance(loc, spatial.wkt($p0))", rql)
20+
self.assertNotIn("spatial.distance(loc),", rql)
21+
22+
def test_distance_ascending_wkt_matches_descending_structure(self):
23+
asc = _rql(OrderByToken.create_distance_ascending_wkt("loc", "p0", None))
24+
desc = _rql(OrderByToken.create_distance_descending_wkt("loc", "p0", None))
25+
# Identical apart from the trailing descending marker.
26+
self.assertEqual(asc, desc.replace(" desc", ""))
27+
28+
def test_distance_ascending_wkt_with_round_factor(self):
29+
rql = _rql(OrderByToken.create_distance_ascending_wkt("loc", "p0", "p1"))
30+
self.assertIn("spatial.distance(loc, spatial.wkt($p0), $p1)", rql)
31+
32+
33+
if __name__ == "__main__":
34+
unittest.main()

0 commit comments

Comments
 (0)