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

[Bug Fix] Broken graphQL query comparisons #2356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BhaskarChezhiyan
Copy link

Overview

graphQL queries with conditions that have an equal or not equal comparison with 0 as operand value are not evaluated properly.

Reproduce

import vaex
import pandas as pd
from decimal import Decimal

df = vaex.from_pandas(pd.Dataframe(data=[[7]], columns=['dummy'] ))

query = """
query($where: BoolExp!, $limit: Int!){
          df(where: $where) { row(limit: $limit){
                               dummy
                               }
                        }
                  }
"""
variables = {'limit': Decimal('10'), 'where': {'dummy': {'_eq': Decimal('0')}}}
output = df.graphql.execute(query, variable_values=variables)

print(output.errors)

Changes

  1. Added null check to Compare class _eq and _neq checks.
  2. Fixed BooleanCompare class to inherit from Compare class rather than NumberCompare.

2. Fixed BooleanCompare class to inherit from Compare class rather than NumberCompare.
@BhaskarChezhiyan BhaskarChezhiyan changed the title Broken Graphql Query conditions [Bug Fix] Broken graphQL query comparisons Mar 24, 2023
@ddelange
Copy link
Contributor

ddelange commented Aug 30, 2024

hey 👋

in #2331 we are encountering an issue with graphql syntax when upgrading to graphene v3: graphql-python/graphene-tornado@a63677c

graphene-tornado v2 (currently required by vaex-graphql) pins werkzeug to an incompatible version for python 3.12 (incompatible with vaex-ml / tensorflow 2.16 to be precise), forcing us to upgrade to graphene-tornado==3.0.0b2, which unpins werkzeug but also requires graphene v3.

do you have any idea how to fix this one?

    def test_aggregates(df, schema):
        result = schema.execute("""
        {
            df {
                count
                min {
                    x
                    y
                }
                mean {
                    x
                    y
                }
                max {
                    x
                    y
                }
            }
        }
        """)
>       assert not result.errors
E       assert not [GraphQLError("Aggregation.__init__() got an unexpected keyword argument 'df'", locations=[SourceLocation(line=3, column=9)], path=['df'])]
E        +  where [GraphQLError("Aggregation.__init__() got an unexpected keyword argument 'df'", locations=[SourceLocation(line=3, column=9)], path=['df'])] = ExecutionResult(data={'df': None}, errors=[GraphQLError("Aggregation.__init__() got an unexpected keyword argument 'df'", locations=[SourceLocation(line=3, column=9)], path=['df'])]).errors

edit: we ended up staying with graphene-tornado v2, by manually unpinning werkzeug in the 2.6.1 release (see top level setup.py used in CI). we can't do this in the official vaex-graphql release as PyPI will reject direct references. end-users can do this too by adding to their requirements file or pip install command (with quotes):

graphene-tornado @ https://github.com/ddelange/graphene-tornado/archive/refs/heads/2.6.1.unpin-werkzeug.zip

ddelange added a commit to ddelange/vaex that referenced this pull request Aug 30, 2024
…o v2.6.1 release

# for now we leave graphene-tornado / graphene on v2
# ref vaexio#2356 (comment)
# in top level vaex[ci] we added the following (additional) direct reference requirement which adds one commit (ddelange/graphene-tornado@d75f01f) on top of the 2.6.1 release to unpin werkzeug
# "graphene-tornado @ https://github.com/ddelange/graphene-tornado/archive/refs/heads/2.6.1.unpin-werkzeug.zip",
maartenbreddels added a commit that referenced this pull request Sep 11, 2024
* fix[ml]: adjust tests to reflect latest apis of 3rd party libraries (xgboost, lightgbm)

* Build wheels on pull_request

* Maximize wheel build parallellization

* git submodule update --remote --merge packages/vaex-core/vendor/pybind11

* Fix gcc error

* Fix workflow syntax

* Remove redefinition

https://github.com/pybind/pybind11/blob/769fd3b889fef6cddb060f2a0be26aee62b4da05/include/pybind11/pytypes.h#L859

https://github.com/ddelange/vaex/actions/runs/3965609112/jobs/6795506653#step:6:2110

* Disable win32

https://github.com/ddelange/vaex/actions/runs/3965689146/jobs/6795667118#step:6:538

* Remove testing leftovers

* Add upper cap on lightgbm

microsoft/LightGBM#5196 (comment)

* Migrate to mamba-org/setup-micromamba@v1

* Upload release assets on Github (pre)release

* Add missing permission for release assets

* Build cp312 wheels

* Remove setuptools and wheel from pyproject.toml

* Replace imp with importlib

* chore: trigger ci

* ci: upgrade xcode for brew install libomp

* update requirements-ml to comply with the latest veex-ml expectations.

* try to install lightgbm via pip

* fix: only the mini taxi file is on s3 for cost savings

* ci: pin dask<2024.2.0 to get the same hash keys

This version gives different results, although
not a problem in production (it will make your
cache invalid though), for CI we test that we
have stable keys (fingerprints)

* fix: only the mini taxi file is on s3 for cost savings (2)

* ci: skip notebooks that depend on the dataframe server

* ci: skip ci steps in various old python versions

* Bump micromamba and other actions

* ci: specific test hangs on ci+osx

* Bump cibuildwheel, use native arm64 mac runners

* test: log is renamed to log loss

* test: skip lightgbm tests on 36 and 37

* test: skip sklearn tests on 36 and 37

* Update packages/vaex-core/setup.py

Co-authored-by: ddelange <14880945+ddelange@users.noreply.github.com>

* chore: drop python 3.6 and 3.7 support

Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>

* test: skip a failing test for windows

* ci: macOS 11 is retired as of June 28

* ci: always build wheels, but do not publish instead

* ci: try with older micromamba, 1.5.6 seems to sometimes hang

* Install setuptools in wheel.yml

* Add sudo

* Fix windows

* Use sudo only on macos

* Fix empty string evaluating false

* Add .readthedocs.yaml

* Pull submodules

* Try editable rtd install

* Try move editable install to requirements_rtd.txt

* Allow newer sphinx

* Autocancel previous runs in PRs

* Autocancel all jobs

* Sphinx sidebar fix

https://github.com/dmwyatt/sphinx-book-theme/blob/2416ef6cebc3d83019dbc83f25c61b72719cad55/docs/customize/sidebar-primary.md#default-sidebar-elements

* Remove autocancel, doesn't work from forks

* Add cancel-in-progress keys

* Amend to only cancel on PRs

* Add CIBW_TEST_COMMAND

* Disallow numpy v2

* Skip musllinux due to misding blake3 wheels

* Install carco for blake3 source install

* Fix CIBW_ENVIRONMENT

* Add CIBW_TEST_SKIP

* Build wheels with numpy v1

* run tests on 3.10-3.12

* upgrade micromamba

* unpin pytest-asyncio

* try different pin as <2.0 because mamba crashes on that

* Fix dephell and CI hell (ddelange#3)

* allow for multiple fingerprints related to package versions

* explicit dtype=object

* fix pandas issues

* revert graphene upgrade

* force uv to ignore Werkzeug (we do not use it, its a subdependency which has conflicts)

* fix: work with modern scipy/sparse arrays

* skip test for python 3.8

* skip test on windows

* add missing import

* skip some tests conditionally

* tuple comparison fix

* does osx run without sourcing .bash profile?

* skip a few more tests

* only run test on linux

* Try building arm64 wheels on macos-13

* Switch to older Xcode

* Fix readthedocs by manually unpinning werkzeug in the graphene-tornado v2.6.1 release

# for now we leave graphene-tornado / graphene on v2
# ref #2356 (comment)
# in top level vaex[ci] we added the following (additional) direct reference requirement which adds one commit (ddelange/graphene-tornado@d75f01f) on top of the 2.6.1 release to unpin werkzeug
# "graphene-tornado @ https://github.com/ddelange/graphene-tornado/archive/refs/heads/2.6.1.unpin-werkzeug.zip",

* Simplify

* Revert "Simplify"

This reverts commit 91263bc.

* Add back tags event trigger for core wheel upload

* Switch to trusted publishing

* Add trusted publishing for vaex-core

* Exclude windows 3.8 from pythonpackage.yml

* Don't upload vaex-core source distribution

* Disable cp38-win vaex-core wheel build

* Add permission

---------

Co-authored-by: Jovan Veljanoski <jovan.veljanoski@tiqets.com>
Co-authored-by: Maarten A. Breddels <maartenbreddels@gmail.com>
Co-authored-by: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
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