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

refactor: fix mypy errors in the clickhouse backend #2856

Closed
datapythonista opened this issue Jul 13, 2021 · 5 comments
Closed

refactor: fix mypy errors in the clickhouse backend #2856

datapythonista opened this issue Jul 13, 2021 · 5 comments
Assignees
Labels
ci Continuous Integration issues or PRs clickhouse The ClickHouse backend onboarding Issues that can be addressed by someone less familiar with ibis refactor Issues or PRs related to refactoring the codebase

Comments

@datapythonista
Copy link
Contributor

datapythonista commented Jul 13, 2021

xref #2823

Running mypy ibis/backends/clickhouse/ reports 5 errors, related to missing or inconsistent type annotations. We should fix them, and make that command finish in a clean way.

Here there are some sample fixes: #2857

@datapythonista datapythonista added ci Continuous Integration issues or PRs clickhouse The ClickHouse backend onboarding Issues that can be addressed by someone less familiar with ibis labels Jul 13, 2021
@napoles-uach
Copy link
Contributor

/take

@napoles-uach
Copy link
Contributor

napoles-uach commented Jul 22, 2021

So it seems to me that one on the errors is related to a list that is turned into a dictionary.. This is the error message:
ibis/backends/clickhouse/registry.py:698: error: Incompatible types in assignment (expression has type "Dict[AnnotableMeta, Callable[[Any, Any, VarArg(Any)], Any]]", variable has type "List[AnnotableMeta]")

and here is the code:

675 _unsupported_ops = [
676     ops.WindowOp,
677     ops.DecimalPrecision,
678     ops.DecimalScale,
679     ops.BaseConvert,
680     ops.CumulativeSum,
681     ops.CumulativeMin,
682     ops.CumulativeMax,
683     ops.CumulativeMean,
684     ops.CumulativeAny,
685     ops.CumulativeAll,
686     ops.IdenticalTo,
687     ops.RowNumber,
688     ops.DenseRank,
689     ops.MinRank,
690     ops.PercentRank,
691     ops.FirstValue,
692     ops.LastValue,
693     ops.NthValue,
694     ops.Lag,
695     ops.Lead,
696     ops.NTile,
697 ]
698 _unsupported_ops = {k: _raise_error for k in _unsupported_ops}

So I'm not sure if Ii is valid to use a temporal variable, or change the name of the list to avoid the conflict @datapythonista
or what would be a best practice??

@datapythonista
Copy link
Contributor Author

You can rename the first one

@cpcloud cpcloud changed the title CI: Fix mypy errors in the clickhouse backend refactor: fix mypy errors in the clickhouse backend Dec 29, 2021
@cpcloud cpcloud added the refactor Issues or PRs related to refactoring the codebase label Dec 29, 2021
@cpcloud
Copy link
Member

cpcloud commented Jan 13, 2022

Rolling this up into #2823. Thanks @napoles-uach for the PR!

@cpcloud cpcloud closed this as completed Jan 13, 2022
@napoles-uach
Copy link
Contributor

My pleasure!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration issues or PRs clickhouse The ClickHouse backend onboarding Issues that can be addressed by someone less familiar with ibis refactor Issues or PRs related to refactoring the codebase
Projects
None yet
Development

No branches or pull requests

3 participants