Skip to content

Commit a7fc706

Browse files
authored
fix: Index on JSON _geq and _leq null filter ops (#4415)
## Relevant issue(s) Resolves #4398 ## Description Use index only for top-level `_leq: null` filter. Other cases like json nested `_leq` or any `_geq` are ignore by index as there is no benefit of using index there. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? Integration tests Specify the platform(s) on which this was tested: - MacOS
1 parent 74b4709 commit a7fc706

10 files changed

Lines changed: 607 additions & 14 deletions

File tree

cli/utils.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,10 +184,10 @@ func setContextRootDir(cmd *cobra.Command) error {
184184
func openKeyring(cmd *cobra.Command) (keyring.Keyring, error) {
185185
cfg := mustGetContextConfig(cmd)
186186
backend := cfg.Get("keyring.backend")
187-
if backend == "system" {
187+
if backend == keyring.KeyringBackendSystem {
188188
return keyring.OpenSystemKeyring(cfg.GetString("keyring.namespace")), nil
189189
}
190-
if backend != "file" {
190+
if backend != keyring.KeyringBackendFile {
191191
log.Info("keyring defaulted to file backend")
192192
}
193193
path := cfg.GetString("keyring.path")

cli/wizard/callbacks.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@ func callback_SetKeyringBackend(s step, ctx *WizardContext) error {
3535
return NewErrModelTypeMismatch(s.ID(), "*modelMultipleChoice")
3636
}
3737

38-
choice := "file"
38+
choice := keyring.KeyringBackendFile
3939
if mm.cursor == 1 {
40-
choice = "system"
40+
choice = keyring.KeyringBackendSystem
4141
}
4242

4343
return setConfigValue(ctx, "keyring.backend", choice)

cli/wizard/unit_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func Test_SetKeyringBackend(t *testing.T) {
7171
if !ok {
7272
t.Fatal("failed to type assert keyring.backend value")
7373
}
74-
if checkedValue != "file" {
74+
if checkedValue != keyring.KeyringBackendFile {
7575
t.Fatal("keyring.backend is not set to file")
7676
}
7777

@@ -84,7 +84,7 @@ func Test_SetKeyringBackend(t *testing.T) {
8484
if !ok {
8585
t.Fatal("failed to type assert keyring.backend value")
8686
}
87-
if checkedValue != "system" {
87+
if checkedValue != keyring.KeyringBackendSystem {
8888
t.Fatal("keyring.backend is not set to system")
8989
}
9090
}
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# Secondary Index geq and leq on null
2+
3+
Fix handling of `_geq` and `_leq` filters on null values with secondary indexes. Tests have been updated to include additional documents to improve coverage of edge cases with null values in JSON fields.

internal/db/fetcher/indexer_iterators.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,15 @@ func (f *indexFetcher) determineFieldFilterConditions() ([]fieldFilterCond, erro
762762
return true
763763
}
764764

765+
// For _geq: null, every value is >= null, so the index provides no benefit -
766+
// we need all documents anyway. Fall back to a full scan.
767+
// For _leq: null on nested JSON paths, documents where the JSON field is entirely
768+
// missing or null should also match. If path is specified though, the index
769+
// has no efficient way of distinguishing between them. Fall back to scan.
770+
if filterVal == nil && (op == opGe || (op == opLe && len(jsonPath) > 0)) {
771+
return true
772+
}
773+
765774
cond, err := makeFieldFilterCondition(op, jsonPath, indexedField, filterVal)
766775

767776
if err != nil {

keyring/keyring.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,16 @@
1010

1111
package keyring
1212

13+
const (
14+
KeyringBackendSystem = "system"
15+
KeyringBackendFile = "file"
16+
)
17+
1318
// Keyring provides a simple set/get interface for a keyring service.
1419
type Keyring interface {
1520
// Set stores the given key in the keystore under the given name.
1621
//
17-
// If a key with the given name already exists it will be overriden.
22+
// If a key with the given name already exists it will be overridden.
1823
Set(name string, key []byte) error
1924
// Get returns the key with the given name from the keystore.
2025
//

tests/integration/index/json_test.go

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1717,3 +1717,257 @@ func TestJSONIndex_WithGreaterThanFilterOnTopLevelJSONField_ShouldUseIndex(t *te
17171717

17181718
testUtils.ExecuteTestCase(t, test)
17191719
}
1720+
1721+
func TestJSONIndex_WithGeqNullFilterOnTopLevelJSONField_ShouldNotUseIndex(t *testing.T) {
1722+
req := `query {
1723+
User(filter: {custom: {_geq: null}}) {
1724+
name
1725+
}
1726+
}`
1727+
1728+
test := testUtils.TestCase{
1729+
Actions: []any{
1730+
&action.AddSchema{
1731+
Schema: `
1732+
type User {
1733+
name: String
1734+
custom: JSON @index
1735+
}
1736+
`,
1737+
},
1738+
&action.CreateDoc{
1739+
Doc: `{
1740+
"name": "John",
1741+
"custom": 21
1742+
}`,
1743+
},
1744+
&action.CreateDoc{
1745+
Doc: `{
1746+
"name": "David",
1747+
"custom": {"age": 32}
1748+
}`,
1749+
},
1750+
&action.CreateDoc{
1751+
Doc: `{
1752+
"name": "Bruno",
1753+
"custom": null
1754+
}`,
1755+
},
1756+
&action.CreateDoc{
1757+
Doc: `{
1758+
"name": "Andy"
1759+
}`,
1760+
},
1761+
&action.Request{
1762+
Request: req,
1763+
Results: map[string]any{
1764+
"User": []map[string]any{
1765+
{"name": "John"},
1766+
{"name": "David"},
1767+
{"name": "Bruno"},
1768+
{"name": "Andy"},
1769+
},
1770+
},
1771+
NonOrderedResults: true,
1772+
},
1773+
&action.Request{
1774+
Request: makeExplainQuery(req),
1775+
Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0),
1776+
},
1777+
},
1778+
}
1779+
1780+
testUtils.ExecuteTestCase(t, test)
1781+
}
1782+
1783+
func TestJSONIndex_WithGeqNullFilterOnNestedJSONPath_ShouldNotUseIndex(t *testing.T) {
1784+
req := `query {
1785+
User(filter: {custom: {age: {_geq: null}}}) {
1786+
name
1787+
}
1788+
}`
1789+
1790+
test := testUtils.TestCase{
1791+
Actions: []any{
1792+
&action.AddSchema{
1793+
Schema: `
1794+
type User {
1795+
name: String
1796+
custom: JSON @index
1797+
}
1798+
`,
1799+
},
1800+
&action.CreateDoc{
1801+
Doc: `{
1802+
"name": "John",
1803+
"custom": {"age": 21}
1804+
}`,
1805+
},
1806+
&action.CreateDoc{
1807+
Doc: `{
1808+
"name": "David",
1809+
"custom": {"age": null}
1810+
}`,
1811+
},
1812+
&action.CreateDoc{
1813+
Doc: `{
1814+
"name": "Bruno"
1815+
}`,
1816+
},
1817+
&action.CreateDoc{
1818+
Doc: `{
1819+
"name": "Andy",
1820+
"custom": {"height": 180}
1821+
}`,
1822+
},
1823+
&action.Request{
1824+
Request: req,
1825+
Results: map[string]any{
1826+
"User": []map[string]any{
1827+
{"name": "John"},
1828+
{"name": "David"},
1829+
{"name": "Bruno"},
1830+
{"name": "Andy"},
1831+
},
1832+
},
1833+
NonOrderedResults: true,
1834+
},
1835+
&action.Request{
1836+
Request: makeExplainQuery(req),
1837+
Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0),
1838+
},
1839+
},
1840+
}
1841+
1842+
testUtils.ExecuteTestCase(t, test)
1843+
}
1844+
1845+
func TestJSONIndex_WithLeqNullFilterOnTopLevelJSONField_ShouldUseIndex(t *testing.T) {
1846+
req := `query {
1847+
User(filter: {custom: {_leq: null}}) {
1848+
name
1849+
}
1850+
}`
1851+
1852+
test := testUtils.TestCase{
1853+
Actions: []any{
1854+
&action.AddSchema{
1855+
Schema: `
1856+
type User {
1857+
name: String
1858+
custom: JSON @index
1859+
}
1860+
`,
1861+
},
1862+
&action.CreateDoc{
1863+
Doc: `{
1864+
"name": "John",
1865+
"custom": 21
1866+
}`,
1867+
},
1868+
&action.CreateDoc{
1869+
Doc: `{
1870+
"name": "David",
1871+
"custom": {"age": 32}
1872+
}`,
1873+
},
1874+
&action.CreateDoc{
1875+
Doc: `{
1876+
"name": "Shahzad",
1877+
"custom": {"age": null}
1878+
}`,
1879+
},
1880+
&action.CreateDoc{
1881+
Doc: `{
1882+
"name": "Bruno",
1883+
"custom": null
1884+
}`,
1885+
},
1886+
&action.CreateDoc{
1887+
Doc: `{
1888+
"name": "Andy"
1889+
}`,
1890+
},
1891+
&action.Request{
1892+
Request: req,
1893+
Results: map[string]any{
1894+
"User": []map[string]any{
1895+
{"name": "Bruno"},
1896+
{"name": "Andy"},
1897+
},
1898+
},
1899+
NonOrderedResults: true,
1900+
},
1901+
&action.Request{
1902+
Request: makeExplainQuery(req),
1903+
Asserter: testUtils.NewExplainAsserter().WithIndexFetches(2),
1904+
},
1905+
},
1906+
}
1907+
1908+
testUtils.ExecuteTestCase(t, test)
1909+
}
1910+
1911+
func TestJSONIndex_WithLeqNullFilterOnNestedJSONPath_ShouldNotUseIndex(t *testing.T) {
1912+
// _leq: null on nested paths matches documents where the path evaluates to null
1913+
// This includes: explicit null at the path, missing path, missing JSON field entirely
1914+
// The index can't efficiently handle all these cases, so it falls back to full scan
1915+
req := `query {
1916+
User(filter: {custom: {age: {_leq: null}}}) {
1917+
name
1918+
}
1919+
}`
1920+
1921+
test := testUtils.TestCase{
1922+
Actions: []any{
1923+
&action.AddSchema{
1924+
Schema: `
1925+
type User {
1926+
name: String
1927+
custom: JSON @index
1928+
}
1929+
`,
1930+
},
1931+
&action.CreateDoc{
1932+
Doc: `{
1933+
"name": "John",
1934+
"custom": {"age": 21}
1935+
}`,
1936+
},
1937+
&action.CreateDoc{
1938+
Doc: `{
1939+
"name": "David",
1940+
"custom": {"age": null}
1941+
}`,
1942+
},
1943+
&action.CreateDoc{
1944+
Doc: `{
1945+
"name": "Bruno"
1946+
}`,
1947+
},
1948+
&action.CreateDoc{
1949+
Doc: `{
1950+
"name": "Andy",
1951+
"custom": {"height": 180}
1952+
}`,
1953+
},
1954+
&action.Request{
1955+
Request: req,
1956+
Results: map[string]any{
1957+
"User": []map[string]any{
1958+
{"name": "David"},
1959+
{"name": "Bruno"},
1960+
{"name": "Andy"},
1961+
},
1962+
},
1963+
NonOrderedResults: true,
1964+
},
1965+
&action.Request{
1966+
Request: makeExplainQuery(req),
1967+
Asserter: testUtils.NewExplainAsserter().WithIndexFetches(0),
1968+
},
1969+
},
1970+
}
1971+
1972+
testUtils.ExecuteTestCase(t, test)
1973+
}

0 commit comments

Comments
 (0)