Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(oracle): use case statements to grab table metadata instead of bools #9379

Closed
wants to merge 5 commits into from

Conversation

gforsyth
Copy link
Member

xref: #9179 #9378

Definitely more fixes needed, I just didn't want to lose track of this branch.

I was able to get some sense of the failures by switching the Oracle docker container:

       - druid
 
   oracle:
-    image: gvenzl/oracle-free:23.4-slim
+    image: gvenzl/oracle-xe:18.4.0-slim
     environment:
       ORACLE_PASSWORD: ibis
       ORACLE_DATABASE: IBIS_TESTING

I couldn't find a version 19 container.
Also note that this requires a bunch of annoying changes to the conftest setup because Oracle 18 doesn't support DROP TABLE IF EXISTS

this is required for Oracle<23c
This is an insufficient fix, as it misses bools in nested projections.
This should probably be a rewrite rule instead, but I'm including it for reference.
ibis/backends/oracle/__init__.py Outdated Show resolved Hide resolved
ibis/backends/oracle/__init__.py Outdated Show resolved Hide resolved
ibis/backends/oracle/__init__.py Outdated Show resolved Hide resolved
ibis/backends/oracle/__init__.py Outdated Show resolved Hide resolved
ibis/backends/oracle/compiler.py Outdated Show resolved Hide resolved
Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>
@cpcloud
Copy link
Member

cpcloud commented Jun 21, 2024

Started playing around with this some more and the outlook isn't great.

I was able to get the test setup working on 18c, but that's the tip of the iceberg.

Here are some things we'd have to do to get this to work:

  1. The conversion of boolean -> CASE-with-int needs to happen in subqueries, not just the final outer query, otherwise the database errors with this beautiful error message

oracledb.exceptions.DatabaseError: ORA-00923: FROM keyword not found where expected

which has effectively nothing to do with what the problem is.

getting this right has proved to be elusive for MS SQL which in 2024 still does not have boolean type 😮‍💨

  1. The bool conversion also has to handle the null case (which isn't a huge deal)
  2. DROP has to execute some PL/SQL, but only in the case that force=True, yay.

My vote is to scrap this PR and only support Oracle starting at version v${THE_ONE_WITH_PROPER_BOOLS}.

@gforsyth
Copy link
Member Author

My vote is to scrap this PR and only support Oracle starting at version v${THE_ONE_WITH_PROPER_BOOLS}.

That is definitely OK with me.

@gforsyth gforsyth closed this Jun 21, 2024
@gitgud5000
Copy link

gitgud5000 commented Jun 22, 2024

I'm using an Oracle 19c db and this fix solved the error I was getting when using con.table():

oracledb.exceptions.DatabaseError: ORA-00923: FROM keyword not found where expected

@gforsyth @cpcloud
Will this fix be available in the next release or will you only officially support >=23c?

edit:
although this fixes being able to instatiate con.table, NUMERIC types are always converted to ints if no precision and scales are set.

When these parameters are set in the DB, I get the following error

---------------------------------------------------------------------------
ArrowInvalid                              Traceback (most recent call last)
File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/IPython/core/formatters.py:711, in PlainTextFormatter.__call__(self, obj)
    704 stream = StringIO()
    705 printer = pretty.RepresentationPrinter(stream, self.verbose,
    706     self.max_width, self.newline,
    707     max_seq_length=self.max_seq_length,
    708     singleton_pprinters=self.singleton_printers,
    709     type_pprinters=self.type_printers,
    710     deferred_pprinters=self.deferred_printers)
--> 711 printer.pretty(obj)
    712 printer.flush()
    713 return stream.getvalue()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/IPython/lib/pretty.py:411, in RepresentationPrinter.pretty(self, obj)
    408                         return meth(obj, self, cycle)
    409                 if cls is not object \
    410                         and callable(cls.__dict__.get('__repr__')):
--> 411                     return _repr_pprint(obj, self, cycle)
    413     return _default_pprint(obj, self, cycle)
    414 finally:

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/IPython/lib/pretty.py:779, in _repr_pprint(obj, p, cycle)
    777 """A pprint that just redirects to the normal repr function."""
    778 # Find newlines and replace them with p.break_()
--> 779 output = repr(obj)
    780 lines = output.splitlines()
    781 with p.group():

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/core.py:80, in Expr.__repr__(self)
     78 def __repr__(self) -> str:
     79     if ibis.options.interactive:
---> 80         return self._interactive_repr()
     81     else:
     82         return self._noninteractive_repr()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/core.py:67, in Expr._interactive_repr(self)
     65 with console.capture() as capture:
     66     try:
---> 67         console.print(self)
     68     except TranslationError as e:
     69         lines = [
     70             "Translation to backend failed",
     71             f"Error message: {e!r}",
     72             "Expression repr follows:",
     73             self._noninteractive_repr(),
     74         ]

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/rich/console.py:1700, in Console.print(self, sep, end, style, justify, overflow, no_wrap, emoji, markup, highlight, width, height, crop, soft_wrap, new_line_start, *objects)
   1698 if style is None:
   1699     for renderable in renderables:
-> 1700         extend(render(renderable, render_options))
   1701 else:
   1702     for renderable in renderables:

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/rich/console.py:1312, in Console.render(self, renderable, options)
   1310 renderable = rich_cast(renderable)
   1311 if hasattr(renderable, "__rich_console__") and not isclass(renderable):
-> 1312     render_iterable = renderable.__rich_console__(self, _options)  # type: ignore[union-attr]
   1313 elif isinstance(renderable, str):
   1314     text_renderable = self.render_str(
   1315         renderable, highlight=_options.highlight, markup=_options.markup
   1316     )

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/core.py:118, in Expr.__rich_console__(self, console, options)
    102 except Exception as e:
    103     # In IPython exceptions inside of _repr_mimebundle_ are swallowed to
    104     # allow calling several display functions and choosing to display
   (...)
    115     #
    116     # This restriction is only present in IPython, not in other REPLs.
    117     console.print_exception()
--> 118     raise e
    119 return console.render(rich_object, options=options)

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/core.py:99, in Expr.__rich_console__(self, console, options)
     97 try:
     98     if opts.interactive:
---> 99         rich_object = to_rich(self, console_width=console_width)
    100     else:
    101         rich_object = Text(self._noninteractive_repr())

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/pretty.py:273, in to_rich(expr, max_rows, max_columns, max_length, max_string, max_depth, console_width)
    269     return _to_rich_scalar(
    270         expr, max_length=max_length, max_string=max_string, max_depth=max_depth
    271     )
    272 else:
--> 273     return _to_rich_table(
    274         expr,
    275         max_rows=max_rows,
    276         max_columns=max_columns,
    277         max_length=max_length,
    278         max_string=max_string,
    279         max_depth=max_depth,
    280         console_width=console_width,
    281     )

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/pretty.py:345, in _to_rich_table(tablish, max_rows, max_columns, max_length, max_string, max_depth, console_width)
    342     if orig_ncols > len(computed_cols):
    343         table = table.select(*computed_cols)
--> 345 result = table.limit(max_rows + 1).to_pyarrow()
    346 # Now format the columns in order, stopping if the console width would
    347 # be exceeded.
    348 col_info = []

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/expr/types/core.py:486, in Expr.to_pyarrow(self, params, limit, **kwargs)
    458 @experimental
    459 def to_pyarrow(
    460     self,
   (...)
    464     **kwargs: Any,
    465 ) -> pa.Table:
    466     """Execute expression and return results in as a pyarrow table.
    467 
    468     This method is eager and will execute the associated expression
   (...)
    484         A pyarrow table holding the results of the executed expression.
    485     """
--> 486     return self._find_backend(use_default=True).to_pyarrow(
    487         self, params=params, limit=limit, **kwargs
    488     )

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/backends/__init__.py:220, in _FileIOHandler.to_pyarrow(self, expr, params, limit, **kwargs)
    216 arrow_schema = schema.to_pyarrow()
    217 with self.to_pyarrow_batches(
    218     table_expr, params=params, limit=limit, **kwargs
    219 ) as reader:
--> 220     table = pa.Table.from_batches(reader, schema=arrow_schema)
    222 return expr.__pyarrow_result__(
    223     table.rename_columns(table_expr.columns).cast(arrow_schema)
    224 )

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/table.pxi:4104, in pyarrow.lib.Table.from_batches()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/ipc.pxi:666, in pyarrow.lib.RecordBatchReader.__next__()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/ipc.pxi:700, in pyarrow.lib.RecordBatchReader.read_next_batch()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/types.pxi:88, in pyarrow.lib._datatype_to_pep3118()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/ibis/backends/sql/__init__.py:375, in <genexpr>(.0)
    372 schema = expr.as_table().schema()
    373 array_type = schema.as_struct().to_pyarrow()
    374 arrays = (
--> 375     pa.array(map(tuple, batch), type=array_type)
    376     for batch in self._cursor_batches(
    377         expr, params=params, limit=limit, chunk_size=chunk_size
    378     )
    379 )
    380 batches = map(pa.RecordBatch.from_struct_array, arrays)
    382 return pa.ipc.RecordBatchReader.from_batches(schema.to_pyarrow(), batches)

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/array.pxi:344, in pyarrow.lib.array()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/array.pxi:42, in pyarrow.lib._sequence_to_array()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/error.pxi:154, in pyarrow.lib.pyarrow_internal_check_status()

File /anaconda/envs/nptb_bpyn/lib/python3.11/site-packages/pyarrow/error.pxi:91, in pyarrow.lib.check_status()

ArrowInvalid: Could not convert Decimal('5.0999999999999996') with type decimal.Decimal: tried to convert to double

@gforsyth
Copy link
Member Author

Will this fix be available in the next release or will you only officially support >=23c?

Hey @gitgud5000 -- I think we currently have to limit ourselves to only supporting >=23c. While some of the fixes in here get you further along using Ibis with Oracle 19 (as you have discovered), there are many remaining pitfalls, especially around query correctness, that need to be addressed due to the lack of proper bool support (among other things).

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