nebius/SWE-rebench-V2-PRs huggingface dataset:
Repository: casbin/casbin
Pull request #1117 (2022-10-11 16:47:43)

feat: fix sub hierarchy invalid in `loadFilteredPolicy`
`SortPoliciesBySubjectHierarchy()` should be executed at `loadFilteredPolicy()` just like `LoadPolicy()`.

casbin-bot:  @tangyang9464 @JalinWang please review
imp2002: The CI error seem be due to a temporary network issue.
![image](https://user-images.githubusercontent.com/41513919/195153772-48133595-3ed7-41f3-ae69-ae76aebed764.png)




hsluoyz: @imp2002 which issue does this PR fix?
imp2002: There is no issue about it, but maybe it should work in `loadFilteredPolicy` too.
[load-policy-with-priority-based-on-role-and-user-hierarchy](https://casbin.io/docs/priority-model#load-policy-with-priority-based-on-role-and-user-hierarchy)

Patch:
diff --git a/enforcer.go b/enforcer.go
index 8d40b7f..de25375 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -353,6 +353,10 @@ func (e *Enforcer) loadFilteredPolicy(filter interface{}) error {
 		return err
 	}
 
+	if err := e.model.SortPoliciesBySubjectHierarchy(); err != nil {
+		return err
+	}
+
 	if err := e.model.SortPoliciesByPriority(); err != nil {
 		return err
 	}


Test patch:
diff --git a/enforcer_test.go b/enforcer_test.go
index 7c35608..6c22020 100644
--- a/enforcer_test.go
+++ b/enforcer_test.go
@@ -531,6 +531,25 @@ func TestSubjectPriorityWithDomain(t *testing.T) {
 	})
 }
 
+func TestSubjectPriorityInFilter(t *testing.T) {
+	e, _ := NewEnforcer()
+
+	adapter := fileadapter.NewFilteredAdapter("examples/subject_priority_policy_with_domain.csv")
+	_ = e.InitWithAdapter("examples/subject_priority_model_with_domain.conf", adapter)
+	if err := e.loadFilteredPolicy(&fileadapter.Filter{
+		P: []string{"", "", "domain1"},
+	}); err != nil {
+		t.Errorf("unexpected error in LoadFilteredPolicy: %v", err)
+	}
+
+	testBatchEnforce(t, e, [][]interface{}{
+		{"alice", "data1", "domain1", "write"},
+		{"admin", "data1", "domain1", "write"},
+	}, []bool{
+		true, false,
+	})
+}
+
 func TestMultiplePolicyDefinitions(t *testing.T) {
 	e, _ := NewEnforcer("examples/multiple_policy_definitions_model.conf", "examples/multiple_policy_definitions_policy.csv")
 	enforceContext := NewEnforceContext("2")
Repository: casbin/casbin
Pull request #1190 (2023-02-04 05:24:29)

feat: add GetAllRolesByDomain() API
Fix: https://github.com/casbin/casbin/issues/1189

casbin-bot:  @tangyang9464 @JalinWang please review

Patch:
diff --git a/rbac_api_with_domains.go b/rbac_api_with_domains.go
index 04944f3..be65a14 100644
--- a/rbac_api_with_domains.go
+++ b/rbac_api_with_domains.go
@@ -144,3 +144,24 @@ func (e *Enforcer) DeleteDomains(domains ...string) (bool, error) {
 func (e *Enforcer) GetAllDomains() ([]string, error) {
 	return e.model["g"]["g"].RM.GetAllDomains()
 }
+
+// GetAllRolesByDomain would get all roles associated with the domain.
+// note: Not applicable to Domains with inheritance relationship  (implicit roles)
+func (e *Enforcer) GetAllRolesByDomain(domain string) []string {
+	g := e.model["g"]["g"]
+	policies := g.Policy
+	roles := make([]string, 0)
+	existMap := make(map[string]bool) // remove duplicates
+
+	for _, policy := range policies {
+		if policy[len(policy)-1] == domain {
+			role := policy[len(policy)-2]
+			if _, ok := existMap[role]; !ok {
+				roles = append(roles, role)
+				existMap[role] = true
+			}
+		}
+	}
+
+	return roles
+}


Test patch:
diff --git a/rbac_api_with_domains_test.go b/rbac_api_with_domains_test.go
index d827c59..b1df57e 100644
--- a/rbac_api_with_domains_test.go
+++ b/rbac_api_with_domains_test.go
@@ -267,3 +267,23 @@ func TestGetAllDomains(t *testing.T) {
 
 	testGetAllDomains(t, e, []string{"domain1", "domain2"})
 }
+
+func testGetAllRolesByDomain(t *testing.T, e *Enforcer, domain string, expected []string) {
+	if !util.SetEquals(e.GetAllRolesByDomain(domain), expected) {
+		t.Errorf("roles in %s: %v, supposed to be %v\n", domain, e.GetAllRolesByDomain(domain), expected)
+	}
+}
+
+func TestGetAllRolesByDomain(t *testing.T) {
+	e, _ := NewEnforcer("examples/rbac_with_domains_model.conf", "examples/rbac_with_domains_policy.csv")
+
+	testGetAllRolesByDomain(t, e, "domain1", []string{"admin"})
+	testGetAllRolesByDomain(t, e, "domain2", []string{"admin"})
+
+	e, _ = NewEnforcer("examples/rbac_with_domains_model.conf", "examples/rbac_with_domains_policy2.csv")
+
+	testGetAllRolesByDomain(t, e, "domain1", []string{"admin"})
+	testGetAllRolesByDomain(t, e, "domain2", []string{"admin"})
+	testGetAllRolesByDomain(t, e, "domain3", []string{"user"})
+
+}
Repository: casbin/casbin
Pull request #1223 (2023-04-04 16:37:25)

fix: fix keymatch with rbac in domain
Fix: #1204 

![image](https://user-images.githubusercontent.com/41513919/229859100-9ac0630b-f0b3-4a3c-b87c-207f7bb1ac0a.png)


hsluoyz: @longsunli plz review

Patch:
diff --git a/enforcer.go b/enforcer.go
index f53739b..277ce42 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -429,6 +429,10 @@ func (e *Enforcer) initRmMap() {
 			_ = rm.Clear()
 		} else {
 			e.rmMap[ptype] = defaultrolemanager.NewRoleManager(10)
+			matchFun := "keyMatch(r_dom, p_dom)"
+			if strings.Contains(e.model["m"]["m"].Value, matchFun) {
+				e.AddNamedDomainMatchingFunc(ptype, "g", util.KeyMatch)
+			}
 		}
 	}
 }
diff --git a/examples/keymatch_with_rbac_in_domain.conf b/examples/keymatch_with_rbac_in_domain.conf
new file mode 100644
index 0000000..396fb45
--- /dev/null
+++ b/examples/keymatch_with_rbac_in_domain.conf
@@ -0,0 +1,14 @@
+[request_definition]
+r = sub, dom, obj, act
+
+[policy_definition]
+p = sub, dom, obj, act
+
+[role_definition]
+g = _, _, _
+
+[policy_effect]
+e = some(where (p.eft == allow))
+
+[matchers]
+m = g(r.sub, p.sub, r.dom) && keyMatch(r.dom, p.dom) && keyMatch(r.obj, p.obj) && regexMatch(r.act, p.act)
diff --git a/examples/keymatch_with_rbac_in_domain.csv b/examples/keymatch_with_rbac_in_domain.csv
new file mode 100644
index 0000000..300579f
--- /dev/null
+++ b/examples/keymatch_with_rbac_in_domain.csv
@@ -0,0 +1,6 @@
+g, can_manage, can_use, *
+
+p, can_manage, engines/*, *, (pause)|(resume)
+p, can_use, engines/*, *, (attach)|(detach)
+
+g, Username==test2, can_manage, engines/engine1


Test patch:
diff --git a/enforcer_test.go b/enforcer_test.go
index 6c22020..8111552 100644
--- a/enforcer_test.go
+++ b/enforcer_test.go
@@ -82,6 +82,11 @@ func TestKeyMatchModelInMemory(t *testing.T) {
 	testEnforce(t, e, "cathy", "/cathy_data", "DELETE", false)
 }
 
+func TestKeyMatchWithRBACInDomain(t *testing.T) {
+	e, _ := NewEnforcer("examples/keymatch_with_rbac_in_domain.conf", "examples/keymatch_with_rbac_in_domain.csv")
+	testDomainEnforce(t, e, "Username==test2", "engines/engine1", "*", "attach", true)
+}
+
 func TestKeyMatchModelInMemoryDeny(t *testing.T) {
 	m := model.NewModel()
 	m.AddDef("r", "r", "sub, obj, act")
Repository: casbin/casbin
Pull request #1413 (2024-06-15 14:26:33)

feat: CachedEnforcer does not clean the result cached in memory when call ClearPolicy method
CachedEnforcer does not clean the result cached in memory when call ClearPolicy method

casbin-bot:  @tangyang9464 @JalinWang please review
hsluoyz: @cococolanosugar fix the two errors:

![image](https://github.com/casbin/casbin/assets/3787410/c6a4eb67-9d36-4e98-bea4-ceab4ec53676)

![image](https://github.com/casbin/casbin/assets/3787410/f364965f-4560-4e43-a6d6-7f5f35f6938c)

cococolanosugar: review again please.

Patch:
diff --git a/enforcer_cached.go b/enforcer_cached.go
index 1e72a97..b89bad7 100644
--- a/enforcer_cached.go
+++ b/enforcer_cached.go
@@ -172,3 +172,14 @@ func GetCacheKey(params ...interface{}) (string, bool) {
 	}
 	return key.String(), true
 }
+
+// ClearPolicy clears all policy.
+func (e *CachedEnforcer) ClearPolicy() {
+	if atomic.LoadInt32(&e.enableCache) != 0 {
+		if err := e.cache.Clear(); err != nil {
+			e.logger.LogError(err, "clear cache failed")
+			return
+		}
+	}
+	e.Enforcer.ClearPolicy()
+}


Test patch:
diff --git a/enforcer_cached_test.go b/enforcer_cached_test.go
index 7af7a3c..34da2fa 100644
--- a/enforcer_cached_test.go
+++ b/enforcer_cached_test.go
@@ -57,4 +57,17 @@ func TestCache(t *testing.T) {
 	testEnforceCache(t, e, "bob", "data2", "write", false)
 	testEnforceCache(t, e, "alice", "data2", "read", true)
 	testEnforceCache(t, e, "alice", "data2", "write", true)
+
+	e, _ = NewCachedEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv")
+	testEnforceCache(t, e, "alice", "data1", "read", true)
+	testEnforceCache(t, e, "bob", "data2", "write", true)
+	testEnforceCache(t, e, "alice", "data2", "read", true)
+	testEnforceCache(t, e, "alice", "data2", "write", true)
+
+	e.ClearPolicy()
+
+	testEnforceCache(t, e, "alice", "data1", "read", false)
+	testEnforceCache(t, e, "bob", "data2", "write", false)
+	testEnforceCache(t, e, "alice", "data2", "read", false)
+	testEnforceCache(t, e, "alice", "data2", "write", false)
 }
Repository: segmentio/parquet-go
Pull request #104 (2022-03-17 14:56:10)

Add failing test for checking bloomfilter of a dict encoded column
I don't have a fix yet, but I noticed that bloomfilter checks don't seem to work as expected on dictionary encoded columns. This adds a test to show the unexpected behavior.


Patch:
diff --git a/writer.go b/writer.go
index 1cea895..537089a 100644
--- a/writer.go
+++ b/writer.go
@@ -762,6 +762,12 @@ func (c *writerColumn) flushFilterPages() error {
 		if c.page.filter == nil {
 			c.page.filter = c.newBloomFilterEncoder(numValues)
 		}
+
+		// If there is a dictionary, we need to only write the dictionary.
+		if dict := c.dictionary; dict != nil {
+			return dict.WriteTo(c.page.filter)
+		}
+
 		for _, page := range c.filter {
 			if err := page.WriteTo(c.page.filter); err != nil {
 				return err


Test patch:
diff --git a/writer_test.go b/writer_test.go
index b4ab7a2..470e4c4 100644
--- a/writer_test.go
+++ b/writer_test.go
@@ -527,3 +527,41 @@ func TestWriterGenerateBloomFilters(t *testing.T) {
 		t.Error(err)
 	}
 }
+
+func TestBloomFilterForDict(t *testing.T) {
+	type testStruct struct {
+		A string `parquet:"a,dict"`
+	}
+
+	schema := parquet.SchemaOf(&testStruct{})
+
+	b := bytes.NewBuffer(nil)
+	w := parquet.NewWriter(
+		b,
+		schema,
+		parquet.BloomFilters(parquet.SplitBlockFilter("a")),
+	)
+
+	err := w.Write(&testStruct{A: "test"})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = w.Close()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f, err := parquet.OpenFile(bytes.NewReader(b.Bytes()), int64(b.Len()))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	ok, err := f.RowGroup(0).Column(0).BloomFilter().Check(parquet.ValueOf("test"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !ok {
+		t.Error("bloom filter should have contained 'test'")
+	}
+}
Repository: segmentio/parquet-go
Pull request #269 (2022-07-12 23:39:28)

Add json tag
Add the ability to set the JSON embedded type.


Patch:
diff --git a/schema.go b/schema.go
index e4cac46..ff0f377 100644
--- a/schema.go
+++ b/schema.go
@@ -502,6 +502,9 @@ func makeStructField(f reflect.StructField) structField {
 		case "dict":
 			setEncoding(&RLEDictionary)
 
+		case "json":
+			setNode(JSON())
+
 		case "delta":
 			switch t.Kind() {
 			case reflect.Int, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint32, reflect.Uint64:
diff --git a/writer.go b/writer.go
index fbf77bb..c6aab35 100644
--- a/writer.go
+++ b/writer.go
@@ -893,9 +893,10 @@ func (c *writerColumn) flushFilterPages() error {
 		// If there is a dictionary, it contains all the values that we need to
 		// write to the filter.
 		if dict := c.dictionary; dict != nil {
-			if c.filter.bits == nil {
-				c.resizeBloomFilter(int64(dict.Len()))
-			}
+			// Need to always attempt to resize the filter, as the writer might
+			// be reused after resetting which would have reset the length of
+			// the filter to 0.
+			c.resizeBloomFilter(int64(dict.Len()))
 			if err := c.writePageToFilter(dict.Page()); err != nil {
 				return err
 			}


Test patch:
diff --git a/schema_test.go b/schema_test.go
index ca7ea25..2ee9a9b 100644
--- a/schema_test.go
+++ b/schema_test.go
@@ -107,6 +107,15 @@ func TestSchemaOf(t *testing.T) {
 		required int64 timestamp_millis (TIMESTAMP(isAdjustedToUTC=true,unit=MILLIS));
 		required int64 timestamp_micros (TIMESTAMP(isAdjustedToUTC=true,unit=MICROS));
 	}
+}`,
+		},
+
+		{
+			value: new(struct {
+				Name string `parquet:",json"`
+			}),
+			print: `message {
+	required binary Name (JSON);
 }`,
 		},
 	}
diff --git a/writer_test.go b/writer_test.go
index 4e005ce..e4dbc5c 100644
--- a/writer_test.go
+++ b/writer_test.go
@@ -652,3 +652,33 @@ func TestWriterRepeatedUUIDDict(t *testing.T) {
 		t.Errorf("expected to get UUID %q back out, got %q", inputID, rowbuf[0][0].Bytes())
 	}
 }
+
+func TestWriterResetWithBloomFilters(t *testing.T) {
+	type Test struct {
+		Value string `parquet:"value,dict"`
+	}
+
+	writer := parquet.NewWriter(new(bytes.Buffer),
+		parquet.BloomFilters(
+			parquet.SplitBlockFilter("value"),
+		),
+	)
+
+	if err := writer.Write(&Test{Value: "foo"}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	writer.Reset(new(bytes.Buffer))
+
+	if err := writer.Write(&Test{Value: "bar"}); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+}
Repository: segmentio/parquet-go
Pull request #407 (2022-11-15 14:12:08)

Update decimal string format
parquet-cli prints the string format of decimals as [`DECIMAL(precision,scale)`](https://github.com/apache/parquet-mr/blob/d057b39d93014fe40f5067ee4a33621e65c91552/parquet-column/src/test/java/org/apache/parquet/parser/TestParquetParser.java#L249-L265). Update parquet-go's string format to match.


Patch:
diff --git a/format/parquet.go b/format/parquet.go
index 08c1c98..6ecdd3e 100644
--- a/format/parquet.go
+++ b/format/parquet.go
@@ -134,7 +134,8 @@ type DecimalType struct {
 }
 
 func (t *DecimalType) String() string {
-	return fmt.Sprintf("DECIMAL(%d,%d)", t.Scale, t.Precision)
+	// Matching parquet-cli's decimal string format: https://github.com/apache/parquet-mr/blob/d057b39d93014fe40f5067ee4a33621e65c91552/parquet-column/src/test/java/org/apache/parquet/parser/TestParquetParser.java#L249-L265
+	return fmt.Sprintf("DECIMAL(%d,%d)", t.Precision, t.Scale)
 }
 
 // Time units for logical types.


Test patch:
diff --git a/print_test.go b/print_test.go
index eae2982..6f535f4 100644
--- a/print_test.go
+++ b/print_test.go
@@ -141,14 +141,14 @@ func TestPrintSchema(t *testing.T) {
 		{
 			node: parquet.Group{"cost": parquet.Decimal(0, 9, parquet.Int32Type)},
 			print: `message Test {
-	required int32 cost (DECIMAL(0,9));
+	required int32 cost (DECIMAL(9,0));
 }`,
 		},
 
 		{
 			node: parquet.Group{"cost": parquet.Decimal(0, 18, parquet.Int64Type)},
 			print: `message Test {
-	required int64 cost (DECIMAL(0,18));
+	required int64 cost (DECIMAL(18,0));
 }`,
 		},
Repository: segmentio/parquet-go
Pull request #420 (2022-12-02 10:43:10)

Add missing Close call in WriteRowGroup
WriteRowGroup would previously not call Close on the Rows passed in to CopyRows. This could cause goroutine leaks when reading pages asynchronously.

cc @achille-roussel 


Patch:
diff --git a/writer.go b/writer.go
index 00ac1de..ef76ef4 100644
--- a/writer.go
+++ b/writer.go
@@ -179,7 +179,9 @@ func (w *Writer) WriteRowGroup(rowGroup RowGroup) (int64, error) {
 		return 0, err
 	}
 	w.writer.configureBloomFilters(rowGroup.ColumnChunks())
-	n, err := CopyRows(w.writer, rowGroup.Rows())
+	rows := rowGroup.Rows()
+	defer rows.Close()
+	n, err := CopyRows(w.writer, rows)
 	if err != nil {
 		return n, err
 	}


Test patch:
diff --git a/row_group_test.go b/row_group_test.go
index 4474438..cb85a97 100644
--- a/row_group_test.go
+++ b/row_group_test.go
@@ -2,6 +2,7 @@ package parquet_test
 
 import (
 	"bytes"
+	"io"
 	"reflect"
 	"sort"
 	"testing"
@@ -421,3 +422,27 @@ func TestMergeRowGroups(t *testing.T) {
 		})
 	}
 }
+
+func TestWriteRowGroupClosesRows(t *testing.T) {
+	var rows []*wrappedRows
+	rg := wrappedRowGroup{
+		RowGroup: newPeopleFile([]Person{{}}),
+		rowsCallback: func(r parquet.Rows) parquet.Rows {
+			wrapped := &wrappedRows{Rows: r}
+			rows = append(rows, wrapped)
+			return wrapped
+		},
+	}
+	writer := parquet.NewWriter(io.Discard)
+	if _, err := writer.WriteRowGroup(rg); err != nil {
+		t.Fatal(err)
+	}
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+	for _, r := range rows {
+		if !r.closed {
+			t.Fatal("rows not closed")
+		}
+	}
+}
Repository: segmentio/parquet-go
Pull request #462 (2022-12-22 09:38:06)

Fix SeekToRow on mergedRowGroupRows
When a Read is performed after SeekToRow on mergedRowGroups, the rowIndex is checked against the seek index and advanced until the rowIndex == seek index. Previously, the rowIndex was not advanced in the normal read path, resulting in mistakenly dropping unread rows when advancing the rowIndex.

asubiotto: cc @achille-roussel 
asubiotto: CI should be green now. The issue was returning `n == 0` on `err == io.EOF`.
asubiotto: Friendly ping @achille-roussel (I apologize if you're still on PTO)

Patch:
diff --git a/merge.go b/merge.go
index 451ae1d..734bb44 100644
--- a/merge.go
+++ b/merge.go
@@ -106,6 +106,12 @@ type mergedRowGroupRows struct {
 	schema    *Schema
 }
 
+func (r *mergedRowGroupRows) readInternal(rows []Row) (int, error) {
+	n, err := r.merge.ReadRows(rows)
+	r.rowIndex += int64(n)
+	return n, err
+}
+
 func (r *mergedRowGroupRows) Close() (lastErr error) {
 	r.merge.close()
 	r.rowIndex = 0
@@ -126,14 +132,13 @@ func (r *mergedRowGroupRows) ReadRows(rows []Row) (int, error) {
 		if n > len(rows) {
 			n = len(rows)
 		}
-		n, err := r.merge.ReadRows(rows[:n])
+		n, err := r.readInternal(rows[:n])
 		if err != nil {
 			return 0, err
 		}
-		r.rowIndex += int64(n)
 	}
 
-	return r.merge.ReadRows(rows)
+	return r.readInternal(rows)
 }
 
 func (r *mergedRowGroupRows) SeekToRow(rowIndex int64) error {


Test patch:
diff --git a/merge_test.go b/merge_test.go
index 16c21ea..10e4e37 100644
--- a/merge_test.go
+++ b/merge_test.go
@@ -423,6 +423,67 @@ func TestMergeRowGroupsCursorsAreClosed(t *testing.T) {
 	}
 }
 
+func TestMergeRowGroupsSeekToRow(t *testing.T) {
+	type model struct {
+		A int
+	}
+
+	schema := parquet.SchemaOf(model{})
+	options := []parquet.RowGroupOption{
+		parquet.SortingRowGroupConfig(
+			parquet.SortingColumns(
+				parquet.Ascending(schema.Columns()[0]...),
+			),
+		),
+	}
+
+	rowGroups := make([]parquet.RowGroup, numRowGroups)
+
+	counter := 0
+	for i := range rowGroups {
+		rows := make([]interface{}, 0, rowsPerGroup)
+		for j := 0; j < rowsPerGroup; j++ {
+			rows = append(rows, model{A: counter})
+			counter++
+		}
+		rowGroups[i] = sortedRowGroup(options, rows...)
+	}
+
+	m, err := parquet.MergeRowGroups(rowGroups, options...)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	func() {
+		mergedRows := m.Rows()
+		defer mergedRows.Close()
+
+		rbuf := make([]parquet.Row, 1)
+		cursor := int64(0)
+		for {
+			if err := mergedRows.SeekToRow(cursor); err != nil {
+				t.Fatal(err)
+			}
+
+			if _, err := mergedRows.ReadRows(rbuf); err != nil {
+				if errors.Is(err, io.EOF) {
+					break
+				}
+				t.Fatal(err)
+			}
+			v := model{}
+			if err := schema.Reconstruct(&v, rbuf[0]); err != nil {
+				t.Fatal(err)
+			}
+			if v.A != int(cursor) {
+				t.Fatalf("expected value %d, got %d", cursor, v.A)
+			}
+
+			cursor++
+		}
+	}()
+}
+
 func BenchmarkMergeRowGroups(b *testing.B) {
 	for _, test := range readerTests {
 		b.Run(test.scenario, func(b *testing.B) {
Repository: segmentio/parquet-go
Pull request #474 (2023-02-09 12:07:47)

parquet: respect NullsFirst sorting order in Buffer
This was previously not respected because null ordering was set after the column was created.

cc @achille-roussel 


Patch:
diff --git a/buffer.go b/buffer.go
index 8f8cc76..9e7e1b8 100644
--- a/buffer.go
+++ b/buffer.go
@@ -81,6 +81,11 @@ func (buf *Buffer) configure(schema *Schema) {
 			columnType = dictionary.Type()
 		}
 
+		sortingIndex := searchSortingColumn(sortingColumns, leaf.path)
+		if sortingIndex < len(sortingColumns) && sortingColumns[sortingIndex].NullsFirst() {
+			nullOrdering = nullsGoFirst
+		}
+
 		column := columnType.NewColumnBuffer(columnIndex, bufferCap)
 		switch {
 		case leaf.maxRepetitionLevel > 0:
@@ -90,13 +95,10 @@ func (buf *Buffer) configure(schema *Schema) {
 		}
 		buf.columns = append(buf.columns, column)
 
-		if sortingIndex := searchSortingColumn(sortingColumns, leaf.path); sortingIndex < len(sortingColumns) {
+		if sortingIndex < len(sortingColumns) {
 			if sortingColumns[sortingIndex].Descending() {
 				column = &reversedColumnBuffer{column}
 			}
-			if sortingColumns[sortingIndex].NullsFirst() {
-				nullOrdering = nullsGoFirst
-			}
 			buf.sorted[sortingIndex] = column
 		}
 	})


Test patch:
diff --git a/buffer_test.go b/buffer_test.go
index 3cca6c9..432077d 100644
--- a/buffer_test.go
+++ b/buffer_test.go
@@ -649,6 +649,49 @@ func TestOptionalDictWriteRowGroup(t *testing.T) {
 	}
 }
 
+func TestNullsSortFirst(t *testing.T) {
+	s := parquet.SchemaOf(&TestStruct{})
+
+	str1 := "test1"
+	str2 := "test2"
+	records := []*TestStruct{
+		{A: &str1},
+		{A: nil},
+		{A: &str2},
+	}
+	buf := parquet.NewBuffer(
+		s,
+		parquet.SortingRowGroupConfig(parquet.SortingColumns(parquet.NullsFirst(parquet.Ascending(s.Columns()[0][0])))),
+	)
+	for _, rec := range records {
+		row := s.Deconstruct(nil, rec)
+		_, err := buf.WriteRows([]parquet.Row{row})
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	sort.Sort(buf)
+
+	rows := buf.Rows()
+	defer rows.Close()
+	rowBuf := make([]parquet.Row, len(records))
+	if _, err := rows.ReadRows(rowBuf); err != nil {
+		t.Fatal(err)
+	}
+
+	resultRecords := make([]TestStruct, len(records))
+	for i, r := range rowBuf {
+		if err := s.Reconstruct(&resultRecords[i], r); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	if resultRecords[0].A != nil {
+		t.Fatal("expected null to sort first, but found", resultRecords)
+	}
+}
+
 func generateBenchmarkBufferRows(n int) (*parquet.Schema, []parquet.Row) {
 	model := new(benchmarkRowType)
 	schema := parquet.SchemaOf(model)
Repository: segmentio/parquet-go
Pull request #86 (2022-03-04 16:34:40)

Preserve correct column index in null values
Before this patch, all null values read from an optional column had the `-1` column index, while non-null values had their correct, expected index.


Patch:
diff --git a/page.go b/page.go
index 6015a92..760d327 100644
--- a/page.go
+++ b/page.go
@@ -353,10 +353,14 @@ func (r *optionalPageReader) ReadValues(values []Value) (n int, err error) {
 		r.values = r.page.base.Values()
 	}
 	maxDefinitionLevel := r.page.maxDefinitionLevel
+	columnIndex := ^int16(r.page.Column())
 
 	for n < len(values) && r.offset < len(r.page.definitionLevels) {
 		for n < len(values) && r.offset < len(r.page.definitionLevels) && r.page.definitionLevels[r.offset] != maxDefinitionLevel {
-			values[n] = Value{definitionLevel: r.page.definitionLevels[r.offset]}
+			values[n] = Value{
+				definitionLevel: r.page.definitionLevels[r.offset],
+				columnIndex:     columnIndex,
+			}
 			r.offset++
 			n++
 		}


Test patch:
diff --git a/page_test.go b/page_test.go
index 43eec4f..284a259 100644
--- a/page_test.go
+++ b/page_test.go
@@ -431,3 +431,21 @@ func TestOptionalPageTrailingNulls(t *testing.T) {
 		t.Errorf("wrong number of rows read: got=%d want=%d", len(resultRows), len(rows))
 	}
 }
+
+func TestOptionalPagePreserveIndex(t *testing.T) {
+	schema := parquet.SchemaOf(&testStruct{})
+	buffer := parquet.NewBuffer(schema)
+
+	if err := buffer.WriteRow(schema.Deconstruct(nil, &testStruct{Value: nil})); err != nil {
+		t.Fatal("writing row:", err)
+	}
+
+	row, err := buffer.Rows().ReadRow(nil)
+	if err != nil {
+		t.Fatal("reading rows:", err)
+	}
+
+	if row[0].Column() != 0 {
+		t.Errorf("wrong index: got=%d want=%d", row[0].Column(), 0)
+	}
+}