ibis icon indicating copy to clipboard operation
ibis copied to clipboard

fix(duckdb): return null typed pyarrow arrays and disable creating tables with all null columns in duckdb

Open cpcloud opened this issue 1 year ago • 3 comments

Address all-NULL column handling in the DuckDB backend. null pyarrow Arrays are returned (previously int32), and it is now an error on the Ibis side to create all null columns with create_table in the DuckDB backend. Fixes #9669.

cpcloud avatar Aug 09 '24 13:08 cpcloud

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

github-actions[bot] avatar Aug 09 '24 13:08 github-actions[bot]

@cpcloud The flink backend is failing, it doesn't like the null type column. I got it to xfail by adding something similar to what you added in _register_in_memory_table() for other backends, in the execute() for flink, although maybe it could go higher up in the to_pyarrow() method. Looks like for flink we don't have the _register_in_memory_table() method.

In backends/flink/__init__.py :

    def execute(self, expr: ir.Expr, **kwargs: Any) -> Any:
        """Execute an expression."""
        self._register_udfs(expr)

        table_expr = expr.as_table()

        if null_columns := table_expr.op().schema.null_fields:
            raise exc.IbisTypeError(
                f"{self.name} cannot yet reliably handle `null` typed columns; "
                f"got null typed columns: {null_columns}")

        sql = self.compile(table_expr, **kwargs)
        df = self._table_env.sql_query(sql).to_pandas()

ncclementi avatar Aug 12 '24 15:08 ncclementi

@cpcloud im motivated to help this one across the finish line. If I fix the flink tests is this good to merge? Any thoughts regarding my incline comments?

NickCrews avatar Sep 18 '24 16:09 NickCrews

@NickCrews Can you give this another review pass and/or approve?

cpcloud avatar Oct 22 '24 14:10 cpcloud

@cpcloud I tacked on two commits that I think are improvements, if those pass CI and you are happy, looks great to me to merge. I can then, in a followup PR, tackle adding in the nested null support that I describe in this comment. Thanks!

NickCrews avatar Oct 22 '24 20:10 NickCrews

Actually, hold on, we aren't converting null-typed scalars correctly. Pushing up a fixup commit soon.

NickCrews avatar Oct 22 '24 20:10 NickCrews

OK, with that I think I'm happy.

NickCrews avatar Oct 22 '24 21:10 NickCrews

Moving off the 10.0 milestone.

cpcloud avatar Dec 30 '24 14:12 cpcloud

Thanks @cpcloud this looks great and I'm very psyched this got into 10.0!

NickCrews avatar Jan 21 '25 15:01 NickCrews