add function & test for parsing table_or_uri #1138

marijncv · 2023-02-12T12:24:22Z

Description

Adds a more specific ValueError if a wrong table_or_uri is provided in the write_delta_lake function. It also adds support for a pathlib Path object.

Related Issue(s)

closes Python: mixing up parameters to write_deltalake gives confusing error #1123

Documentation

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

wjones127

Thank you for doing this. Would you be wiling to add Path support to DeltaTable too? Then we can clean up most of the unit tests that have to do the Path to str conversion.

python/tests/test_writer.py

wjones127 · 2023-02-12T17:40:38Z

python/tests/test_writer.py

+    data = pa.table({"vals": pa.array(["1", "2", "3"])})
+    table_or_uri = str(tmp_path / "delta_table")
+    write_deltalake(table_or_uri, data)
+    delta_table = DeltaTable(table_or_uri)


I see. We need DeltaTable.__init__ to support Path too. Would you be willing to add that as well?

Added it there! And also in try_get_deltatable

roeap · 2023-02-12T18:24:52Z

python/deltalake/writer.py

+            # Non-existant local paths are only accepted as fully-qualified URIs
+            table_uri = "file://" + str(Path(table_or_uri).absolute())


by now we actually extended the path handling on the rust side to also support relative paths and scheme-less paths, and also create a directory if it does not yet exists.

delta-rs/rust/src/builder.rs

Lines 353 to 376 in 67b512f

pub(crate) fn ensure_table_uri(table_uri: impl AsRef<str>) -> DeltaResult<Url> {

let table_uri = table_uri.as_ref();

if let Ok(path) = std::fs::canonicalize(table_uri) {

return Url::from_directory_path(path)

.map_err(|_| DeltaTableError::InvalidTableLocation(table_uri.to_string()));

}

if let Ok(url) = Url::parse(table_uri) {

return Ok(match url.scheme() {

"file" => url,

_ => {

let mut new_url = url.clone();

new_url.set_path(url.path().trim_end_matches('/'));

new_url

}

});

}

// The table uri still might be a relative paths that does not exist.

std::fs::create_dir_all(table_uri)

.map_err(|_| DeltaTableError::InvalidTableLocation(table_uri.to_string()))?;

let path = std::fs::canonicalize(table_uri)

.map_err(|_| DeltaTableError::InvalidTableLocation(table_uri.to_string()))?;

Url::from_directory_path(path)

.map_err(|_| DeltaTableError::InvalidTableLocation(table_uri.to_string()))

}

Is it maybe a good idea to rely on the same logic on the python side as well, so we are always consistent in the supported paths?

Of course supporting Path objects will always be python territory :).

Thanks for the pointer! That makes the python code a bit simpler

roeap · 2023-02-12T18:25:59Z

python/deltalake/writer.py

+) -> Tuple[Optional[DeltaTable], str]:
+    """Parses `table_or_uri` and returns `table` & `table_uri`.
+
+    Raises a ValueError if `table_or_uri` is not of type `str`, `Path` or `DeltaTable`


Should we maybe write this in numpy docstring format? https://numpydoc.readthedocs.io/en/latest/format.html#raises

I am very sorry, but I said something wrong. We are not using the numpy format... we are using the reST format. not sure how raises are done there. @wjones127 - do you know? I could not find good docs for that.

Ah yes, the format shown here looks very similar to what is used in other places in the codebase and it also specifies a way to describe a raise. I can rewrite it in that format if it's the correct one to use

Yes that's the format. Could you rewrite that?

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

wjones127

Thanks! This is a great improvement :)

Signed-off-by: Marijn Valk <marijncv@hotmail.com> # Description Now that #1138 is merged, we can use `pathlib.Path` instead of `str` in `write_deltalake()` and `DeltaTable.__init__()` --------- Signed-off-by: Marijn Valk <marijncv@hotmail.com>

# Description Adds a more specific ValueError if a wrong `table_or_uri` is provided in the `write_delta_lake` function. It also adds support for a pathlib `Path` object. # Related Issue(s) - closes delta-io#1123 # Documentation  --------- Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Signed-off-by: Marijn Valk <marijncv@hotmail.com> # Description Now that delta-io#1138 is merged, we can use `pathlib.Path` instead of `str` in `write_deltalake()` and `DeltaTable.__init__()` --------- Signed-off-by: Marijn Valk <marijncv@hotmail.com>

add function & test for parsing table_or_uri

0895e79

marijncv requested review from wjones127, fvaleye, rtyler, roeap and houqp as code owners February 12, 2023 12:24

github-actions bot added the binding/python Issues for the Python package label Feb 12, 2023

marijncv added 2 commits February 12, 2023 13:37

test without pandas

16e7361

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

add pathlib.Path to nitpick_ignore

11d2b77

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

github-actions bot added the documentation Improvements or additions to documentation label Feb 12, 2023

clean up test

d4dfcd9

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

wjones127 reviewed Feb 12, 2023

View reviewed changes

roeap reviewed Feb 12, 2023

View reviewed changes

marijncv added 2 commits February 12, 2023 21:01

extend path support & numpy docstring

cd71a7e

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

use correct docstring format

353db05

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

wjones127 approved these changes Feb 12, 2023

View reviewed changes

wjones127 merged commit fab36eb into delta-io:main Feb 12, 2023

marijncv deleted the parsing-table-or-uri-in-dt-writer branch February 13, 2023 06:43

marijncv mentioned this pull request Feb 13, 2023

use Path object in writer tests #1147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add function & test for parsing table_or_uri #1138

add function & test for parsing table_or_uri #1138

marijncv commented Feb 12, 2023

wjones127 left a comment

wjones127 Feb 12, 2023

marijncv Feb 12, 2023

roeap Feb 12, 2023

marijncv Feb 12, 2023

roeap Feb 12, 2023

roeap Feb 12, 2023

marijncv Feb 12, 2023

wjones127 Feb 12, 2023

wjones127 left a comment

		# Non-existant local paths are only accepted as fully-qualified URIs
		table_uri = "file://" + str(Path(table_or_uri).absolute())

	pub(crate) fn ensure_table_uri(table_uri: impl AsRef<str>) -> DeltaResult<Url> {
	let table_uri = table_uri.as_ref();
	if let Ok(path) = std::fs::canonicalize(table_uri) {
	return Url::from_directory_path(path)
	.map_err(\|_\| DeltaTableError::InvalidTableLocation(table_uri.to_string()));
	}
	if let Ok(url) = Url::parse(table_uri) {
	return Ok(match url.scheme() {
	"file" => url,
	_ => {
	let mut new_url = url.clone();
	new_url.set_path(url.path().trim_end_matches('/'));
	new_url
	}
	});
	}
	// The table uri still might be a relative paths that does not exist.
	std::fs::create_dir_all(table_uri)
	.map_err(\|_\| DeltaTableError::InvalidTableLocation(table_uri.to_string()))?;
	let path = std::fs::canonicalize(table_uri)
	.map_err(\|_\| DeltaTableError::InvalidTableLocation(table_uri.to_string()))?;
	Url::from_directory_path(path)
	.map_err(\|_\| DeltaTableError::InvalidTableLocation(table_uri.to_string()))
	}

add function & test for parsing table_or_uri #1138

add function & test for parsing table_or_uri #1138

Conversation

marijncv commented Feb 12, 2023

Description

Related Issue(s)

Documentation

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment