feat: support merge by row_id, row_addr

[chenkovsky]

No description provided.

[wjones127]

Thanks for working on this @chenkovsky. I would like to see a few improvements to the unit tests, and then this is ready to go.

[wjones127]

If we aren't testing anything about the files, let's use an in-memory dataset instead.

Suggested change

	let test_dir = tempdir().unwrap();
	let test_uri = test_dir.path().to_str().unwrap();

[wjones127]

If you re-use the dataset instance from write(), you can just use an in-memory dataset:

Suggested change

	Dataset::write(data, test_uri, Some(write_params.clone()))
	.await
	.unwrap();
	let mut dataset = Dataset::open(test_uri).await.unwrap();
	let dataset = Dataset::write(data, "memory://", Some(write_params.clone()))
	.await
	.unwrap();

[wjones127]

I'd like us to assert a few more things in this test:

1. dataset has the expected final schema key, value, new_value.
2. The values are what we expect. For this, you should avoid using the same values in each column. Otherwise, the test could pass even if there is a bug that uses the wrong column's values. Right now, you use row_ids.clone() for both rowid and new_value.
3. This works even if you shuffle the data. I would recommend using take_record_batch() to reorder the new_batch so the row ids are out-of-order.

[wjones127]

Where is the null filling? It seems like you are providing every row id, unless I am missing something.

[chenkovsky]

Where is the null filling? It seems like you are providing every row id, unless I am missing something.

sorry, I copy and modify another test

[wjones127]

Same comments from the row id test apply here.

[chenkovsky]

updated

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.88%. Comparing base (83b8efd) to head (b216aa0).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3254      +/-   ##
==========================================
+ Coverage   78.47%   78.88%   +0.41%     
==========================================
  Files         245      246       +1     
  Lines       85088    86568    +1480     
  Branches    85088    86568    +1480     
==========================================
+ Hits        66772    68292    +1520     
+ Misses      15501    15450      -51     
- Partials     2815     2826      +11     

Flag	Coverage Δ
unittests	78.88% <100.00%> (+0.41%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wjones127]

Looks better. Have a few suggestions to clean up the tests. Then I think this is ready to be merged.

[wjones127]

You can use .try_into_batch() to immediately collect the results in one batch. Will need to remove the for loop below too.

Suggested change

	let result = dataset
	.scan()
	.try_into_stream()
	.await
	.unwrap()
	.try_collect::<Vec<_>>()
	.await
	.unwrap();
	let result = dataset
	.scan()
	.try_into_batch()
	.await
	.unwrap();

[wjones127]

For tests, you can use RecordBatch["<column>"] to access a column by name. Also, .as_primitive() is typically shorter. Both of these are mostly suitable for tests because they can panic.

Suggested change

	let key = batch
	.column_by_name("key")
	.unwrap()
	.as_any()
	.downcast_ref::<arrow_array::Int32Array>()
	.unwrap();
	let key = batch["key"].as_primitive::<Int32Type>();