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

feat: add clustername to ALLOWED_GROUP_BY_FIELDS #25

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

Conversation

NoamGaash
Copy link
Member

@NoamGaash NoamGaash commented May 1, 2023

this pr will allow grouping lines by cluster.
for example, grouping all rides belongs the נגב or to the גולן clusters.

@OriHoch
Copy link
Contributor

OriHoch commented May 2, 2023

where did clustertoline table come from? which PR added it?

Copy link
Contributor

@OriHoch OriHoch left a comment

Choose a reason for hiding this comment

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

I opened an issue (hasadna/open-bus-pipelines#7) regarding documenting the clustertoline table or adding it to ETL, but it shouldn't prevent merging this PR

@NoamGaash
Copy link
Member Author

@OriHoch
I'm not authorized to merge this pull request.
This table is manually downloaded from here - https://github.com/hasadna/open-bus-gtfs-etl/blob/main/open_bus_gtfs_etl/gtfs_extractor.py#L49
@ShayAdler thinks this data won't change so often, so for now, it would be good enough. we can do a proper ETL pipeline later

@OriHoch
Copy link
Contributor

OriHoch commented May 3, 2023

@NoamGaash did you test your API changes locally? how did you test it?

@NoamGaash
Copy link
Member Author

NoamGaash commented May 4, 2023

@OriHoch I have tested this SQL query:

select
           operator_ref, clustername,
            count(1) as total_routes,
            sum(agg.num_planned_rides) as total_planned_rides,
            sum(agg.num_actual_rides) as total_actual_rides
        from gtfs_rides_agg agg, gtfs_route rt join clustertoline c on rt.line_ref = c.officelineid
        where
            agg.gtfs_route_id = rt.id
            and agg.gtfs_route_date >= '2023-01-01'
            and agg.gtfs_route_date <= '2023-01-10'
        group by operator_ref, clustername

and it seems to work
image

@OriHoch OriHoch self-requested a review May 5, 2023 04:22
OriHoch

This comment was marked as outdated.

@OriHoch
Copy link
Contributor

OriHoch commented May 5, 2023

There is an error, following is the bug report:

reproduction steps

  • run API method /gtfs_rides_agg/group_by with following arguments:
    • date_from: 2023-04-01
    • date_to: 2023-04-03
    • group_by: clustername

expected

  • result groupeb by clustername

actual

{
  "message": "(psycopg2.errors.UndefinedColumn) column rt.clustername does not exist\nLINE 3:     rt.clustername as clustername,\n            ^\nHINT:  Perhaps you meant to reference the column \"c.clustername\".\n\n[SQL: \nselect\n    rt.clustername as clustername,\n    count(1) as total_routes,\n    sum(agg.num_planned_rides) as total_planned_rides,\n    sum(agg.num_actual_rides) as total_actual_rides\nfrom gtfs_rides_agg agg, gtfs_route rt join clustertoline c on rt.line_ref = c.officelineid\nwhere\n    agg.gtfs_route_id = rt.id\n    and agg.gtfs_route_date >= %(date_from)s\n    and agg.gtfs_route_date <= %(date_to)s\ngroup by rt.clustername\n]\n[parameters: {'date_from': datetime.date(2023, 4, 1), 'date_to': datetime.date(2023, 4, 3)}]\n(Background on this error at: https://sqlalche.me/e/14/f405)",
  "traceback": [
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/middleware/errors.py\", line 162, in __call__\n    await self.app(scope, receive, _send)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/middleware/cors.py\", line 84, in __call__\n    await self.app(scope, receive, send)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/exceptions.py\", line 93, in __call__\n    raise exc\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/exceptions.py\", line 82, in __call__\n    await self.app(scope, receive, sender)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py\", line 21, in __call__\n    raise e\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/fastapi/middleware/asyncexitstack.py\", line 18, in __call__\n    await self.app(scope, receive, send)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/routing.py\", line 670, in __call__\n    await route.handle(scope, receive, send)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/routing.py\", line 266, in handle\n    await self.app(scope, receive, send)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/routing.py\", line 65, in app\n    response = await func(request)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/fastapi/routing.py\", line 227, in app\n    raw_response = await run_endpoint_function(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/fastapi/routing.py\", line 162, in run_endpoint_function\n    return await run_in_threadpool(dependant.call, **values)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/starlette/concurrency.py\", line 41, in run_in_threadpool\n    return await anyio.to_thread.run_sync(func, *args)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/anyio/to_thread.py\", line 31, in run_sync\n    return await get_asynclib().run_sync_in_worker_thread(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/anyio/_backends/_asyncio.py\", line 937, in run_sync_in_worker_thread\n    return await future\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/anyio/_backends/_asyncio.py\", line 867, in run\n    result = context.run(func, *args)\n",
    "  File \"/home/ori/open-bus-stride-api/./open_bus_stride_api/routers/gtfs_rides_agg.py\", line 103, in group_by_\n    return sql_route.list_(sql, sql_params, None, None, None, None, None, True)\n",
    "  File \"/home/ori/open-bus-stride-api/./open_bus_stride_api/common/sql_route.py\", line 38, in list_\n    iterator = (o for o in session.execute(sql, sql_params, execution_options={'stream_results': True}))\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/orm/session.py\", line 1714, in execute\n    result = conn._execute_20(statement, params or {}, execution_options)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py\", line 1705, in _execute_20\n    return meth(self, args_10style, kwargs_10style, execution_options)\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/sql/elements.py\", line 334, in _execute_on_connection\n    return connection._execute_clauseelement(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py\", line 1572, in _execute_clauseelement\n    ret = self._execute_context(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py\", line 1943, in _execute_context\n    self._handle_dbapi_exception(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py\", line 2124, in _handle_dbapi_exception\n    util.raise_(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/util/compat.py\", line 211, in raise_\n    raise exception\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py\", line 1900, in _execute_context\n    self.dialect.do_execute(\n",
    "  File \"/home/ori/open-bus-stride-api/venv/lib/python3.8/site-packages/sqlalchemy/engine/default.py\", line 736, in do_execute\n    cursor.execute(statement, parameters)\n"
  ]
}

@OriHoch OriHoch self-requested a review May 5, 2023 04:30
Copy link
Contributor

@OriHoch OriHoch left a comment

Choose a reason for hiding this comment

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

there is an error - see previous comment for the bug report

please test locally

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.

2 participants