Skip to content

Conversation

@ccmao1130
Copy link
Contributor

Rationale for this change

We recently changed our site domain so want to update all Daft documentation links. And noticed that our package should be updated from getdaft to daft

Are these changes tested?

yes

Are there any user-facing changes?

no

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @ccmao1130, we can move this forward as soon as the CI is green 👍

psycopg2-binary = { version = ">=2.9.6", optional = true }
sqlalchemy = { version = "^2.0.18", optional = true }
getdaft = { version = ">=0.2.12", optional = true }
daft = { version = ">=0.5.0", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating the minor version requires running poetry lock. What's the rationale for bumping to 0.5.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We changed our package from getdaft to daft and the earliest version for daft starts at v0.4.7 but we wanted to bump to v0.5.0

@kevinjqliu
Copy link
Contributor

@ccmao1130 could you run make install and then poetry lock locally? That should resolve the CI issue

@ccmao1130
Copy link
Contributor Author

@kevinjqliu Okay I think it should be okay now? (sorry not a developer hahaha....)

@kevinjqliu
Copy link
Contributor

Looks like something changed in the newer version

This test failed

@pytest.mark.integration
@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive"), pytest.lazy_fixture("session_catalog")])
def test_daft_nan_rewritten(catalog: Catalog) -> None:
import daft
daft.context.set_runner_native()
table_test_null_nan_rewritten = catalog.load_table("default.test_null_nan_rewritten")
df = table_test_null_nan_rewritten.to_daft()
df = df.where(df["col_numeric"].float.is_nan())
df = df.select("idx", "col_numeric")
assert df.count_rows() == 1
assert df.to_pydict()["idx"][0] == 1
assert math.isnan(df.to_pydict()["col_numeric"][0])

___________________ test_daft_nan_rewritten[session_catalog] ___________________

catalog = local (<class 'pyiceberg.catalog.rest.RestCatalog'>)

    @pytest.mark.integration
    @pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive"), pytest.lazy_fixture("session_catalog")])
    def test_daft_nan_rewritten(catalog: Catalog) -> None:
        import daft
    
>       daft.context.set_runner_native()

tests/integration/test_reads.py:338: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

num_threads = None

    def set_runner_native(num_threads: int | None = None) -> DaftContext:
        """Configure Daft to execute dataframes using native multi-threaded processing.
    
        This is the default execution mode for Daft.
    
        Returns:
            DaftContext: Updated Daft execution context configured for native execution.
    
        Note:
            Can also be configured via environment variable: DAFT_RUNNER=native
        """
>       py_ctx = _set_runner_native(num_threads=num_threads)
E       daft.exceptions.DaftCoreException: DaftError::InternalError Cannot set runner more than once

../../../.cache/pypoetry/virtualenvs/pyiceberg-bKWKvoA4-py3.10/lib/python3.10/site-packages/daft/context.py:107: DaftCoreException

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is green now. LGTM!

@kevinjqliu kevinjqliu merged commit 479e663 into apache:main Jul 9, 2025
18 of 19 checks passed
@ccmao1130
Copy link
Contributor Author

Thanks Kevin!

gabeiglio pushed a commit to Netflix/iceberg-python that referenced this pull request Aug 13, 2025
<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

# Rationale for this change

We recently changed our site domain so want to update all Daft
documentation links. And noticed that our package should be updated from
`getdaft` to `daft`

# Are these changes tested?

yes

# Are there any user-facing changes?

no

<!-- In the case of user-facing changes, please add the changelog label.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants