-
Notifications
You must be signed in to change notification settings - Fork 0
TIMX 543 - keyset pagination for read methods #169
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -364,7 +364,8 @@ def read_batches_iter( | |
| ) -> Iterator[pa.RecordBatch]: | ||
| """Yield ETL records as pyarrow.RecordBatches. | ||
|
|
||
| This method performs a two step process: | ||
| This is the base read method. All read methods eventually drop down and use this | ||
| for streaming batches of records. This method performs a two-step process: | ||
|
|
||
| 1. Perform a "metadata" query that narrows down records and physical parquet | ||
| files to read from. | ||
|
|
@@ -383,34 +384,43 @@ def read_batches_iter( | |
| """ | ||
| start_time = time.perf_counter() | ||
|
|
||
| # build and execute metadata query | ||
| metadata_time = time.perf_counter() | ||
| meta_query = self.metadata.build_meta_query(table, limit, where, **filters) | ||
| meta_df = self.metadata.conn.query(meta_query).to_df() | ||
| logger.debug( | ||
| f"Metadata query identified {len(meta_df)} rows, " | ||
| f"across {len(meta_df.filename.unique())} parquet files, " | ||
| f"elapsed: {round(time.perf_counter()-metadata_time,2)}s" | ||
| ) | ||
|
|
||
| # execute data queries in batches and yield results | ||
| temp_table_name = "read_meta_chunk" | ||
| total_yield_count = 0 | ||
| for i, meta_chunk_df in enumerate(self._iter_meta_chunks(meta_df)): | ||
|
|
||
| for i, meta_chunk_df in enumerate( | ||
| self._iter_meta_chunks( | ||
| table, | ||
| limit=limit, | ||
| where=where, | ||
| **filters, | ||
| ) | ||
| ): | ||
| batch_time = time.perf_counter() | ||
| batch_yield_count = len(meta_chunk_df) | ||
| total_yield_count += batch_yield_count | ||
|
|
||
| if batch_yield_count == 0: | ||
| continue | ||
|
|
||
| self.conn.register("meta_chunk", meta_chunk_df) | ||
| data_query = self._build_data_query_for_chunk( | ||
| columns, | ||
| meta_chunk_df, | ||
| registered_metadata_chunk="meta_chunk", | ||
| self.conn.register( | ||
| temp_table_name, | ||
| meta_chunk_df[ | ||
| [ | ||
| "timdex_record_id", | ||
| "run_id", | ||
| "run_record_offset", | ||
| ] | ||
| ], | ||
| ) | ||
| yield from self._stream_data_query_batches(data_query) | ||
| self.conn.unregister("meta_chunk") | ||
|
|
||
| # build and perform data query, yield records | ||
| # set in try/finally block to ensure we always deregister the meta table | ||
| try: | ||
| data_query = self._build_data_query_for_chunk( | ||
| columns, | ||
| meta_chunk_df, | ||
| registered_metadata_chunk=temp_table_name, | ||
| ) | ||
| yield from self._iter_data_chunks(data_query) | ||
| finally: | ||
| self.conn.unregister(temp_table_name) | ||
|
|
||
| batch_rps = int(batch_yield_count / (time.perf_counter() - batch_time)) | ||
| logger.debug( | ||
|
|
@@ -422,32 +432,104 @@ def read_batches_iter( | |
| f"read_batches_iter() elapsed: {round(time.perf_counter()-start_time, 2)}s" | ||
| ) | ||
|
|
||
| def _iter_meta_chunks(self, meta_df: pd.DataFrame) -> Iterator[pd.DataFrame]: | ||
| """Utility method to yield chunks of metadata query results.""" | ||
| for start in range(0, len(meta_df), self.config.duckdb_join_batch_size): | ||
| yield meta_df.iloc[start : start + self.config.duckdb_join_batch_size] | ||
| def _iter_meta_chunks( | ||
| self, | ||
| table: str = "records", | ||
| limit: int | None = None, | ||
| where: str | None = None, | ||
| **filters: Unpack[DatasetFilters], | ||
| ) -> Iterator[pd.DataFrame]: | ||
| """Utility method to yield pandas Dataframe chunks of metadata query results. | ||
|
|
||
| def _build_parquet_file_list(self, meta_chunk_df: pd.DataFrame) -> str: | ||
| """Build SQL list of parquet filepaths.""" | ||
| filenames = meta_chunk_df["filename"].unique().tolist() | ||
| if self.location_scheme == "s3": | ||
| filenames = [f"s3://{f.removeprefix('s3://')}" for f in filenames] | ||
| return "[" + ",".join((f"'{f}'") for f in filenames) + "]" | ||
| The approach here is to use "keyset" pagination, which means each paged result | ||
| is a greater-than (>) check against a tuple of ordered values from the previous | ||
| chunk. This is more performant than a LIMIT + OFFSET. | ||
| """ | ||
| # use duckdb_join_batch_size as the chunk size for keyset pagination | ||
| chunk_size = self.config.duckdb_join_batch_size | ||
|
|
||
| # init keyset value of zeros to begin with | ||
| keyset_value = (0, 0, 0) | ||
|
|
||
| total_yielded = 0 | ||
| while True: | ||
|
|
||
| # enforce limit if passed | ||
| if limit is not None: | ||
| remaining = limit - total_yielded | ||
| if remaining <= 0: | ||
| break | ||
| chunk_limit = min(chunk_size, remaining) | ||
| else: | ||
| chunk_limit = chunk_size | ||
|
|
||
| # perform chunk query and convert to pyarrow Table | ||
| meta_query = self.metadata.build_keyset_paginated_metadata_query( | ||
| table, | ||
| limit=chunk_limit, # pass chunk_limit instead of limit | ||
| where=where, | ||
| keyset_value=keyset_value, | ||
| **filters, | ||
| ) | ||
| meta_chunk_df = self.metadata.conn.query(meta_query).to_df() | ||
|
|
||
| meta_chunk_count = len(meta_chunk_df) | ||
|
|
||
| # an empty chunk signals end of pagination | ||
| if meta_chunk_count == 0: | ||
| break | ||
|
|
||
| # yield this chunk of data | ||
| total_yielded += meta_chunk_count | ||
| yield meta_chunk_df[ | ||
| [ | ||
| "timdex_record_id", | ||
| "run_id", | ||
| "run_record_offset", | ||
| "filename", | ||
| ] | ||
| ] | ||
|
|
||
| # update keyset value using the last row from this chunk | ||
| last_row = meta_chunk_df.iloc[-1] | ||
| keyset_value = ( | ||
| int(last_row.filename_hash), | ||
| int(last_row.run_id_hash), | ||
| int(last_row.run_record_offset), | ||
| ) | ||
|
|
||
| def _build_data_query_for_chunk( | ||
| self, | ||
| columns: list[str] | None, | ||
| meta_chunk_df: pd.DataFrame, | ||
| registered_metadata_chunk: str = "meta_chunk", | ||
| ) -> str: | ||
| """Build SQL query used for data retrieval, joining on metadata data.""" | ||
| parquet_list_sql = self._build_parquet_file_list(meta_chunk_df) | ||
| rro_list_sql = ",".join( | ||
| str(rro) for rro in meta_chunk_df["run_record_offset"].unique() | ||
| ) | ||
| """Build SQL query used for data retrieval, joining on passed metadata data.""" | ||
| # build select columns | ||
| select_cols = ",".join( | ||
| [f"ds.{col}" for col in (columns or TIMDEX_DATASET_SCHEMA.names)] | ||
| ) | ||
|
|
||
| # build list of explicit parquet files to read from | ||
| filenames = list(meta_chunk_df["filename"].unique()) | ||
| if self.location_scheme == "s3": | ||
| filenames = [ | ||
| f"s3://{f.removeprefix('s3://')}" for f in filenames # type: ignore[union-attr] | ||
| ] | ||
| parquet_list_sql = "[" + ",".join(f"'{f}'" for f in filenames) + "]" | ||
|
|
||
| # build run_record_offset WHERE clause to leverage row group pruning | ||
| rro_values = meta_chunk_df["run_record_offset"].unique() | ||
| rro_values.sort() | ||
| if len(rro_values) <= 1_000: # noqa: PLR2004 | ||
| rro_clause = ( | ||
| f"and run_record_offset in ({','.join(str(rro) for rro in rro_values)})" | ||
| ) | ||
| else: | ||
| rro_clause = ( | ||
| f"and run_record_offset between {rro_values[0]} and {rro_values[-1]}" | ||
| ) | ||
|
Comment on lines
+513
to
+531
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very little meaningful change here. This is primarily code from sub-methods that were removed, where the misdirection of jumping around to them felt worse than just having it here. |
||
|
|
||
| return f""" | ||
| select | ||
| {select_cols} | ||
|
|
@@ -459,15 +541,24 @@ def _build_data_query_for_chunk( | |
| inner join {registered_metadata_chunk} mc using ( | ||
| timdex_record_id, run_id, run_record_offset | ||
| ) | ||
| where ds.run_record_offset in ({rro_list_sql}); | ||
| where true | ||
| {rro_clause}; | ||
| """ | ||
|
|
||
| def _stream_data_query_batches(self, data_query: str) -> Iterator[pa.RecordBatch]: | ||
| """Yield pyarrow RecordBatches from a SQL query.""" | ||
| self.conn.execute("set enable_progress_bar = false;") | ||
| cursor = self.conn.execute(data_query) | ||
| yield from cursor.fetch_record_batch(rows_per_batch=self.config.read_batch_size) | ||
| self.conn.execute("set enable_progress_bar = true;") | ||
| def _iter_data_chunks(self, data_query: str) -> Iterator[pa.RecordBatch]: | ||
| """Perform a query to retrieve data and stream chunks.""" | ||
| if self.location_scheme == "s3": | ||
| self.conn.execute("""set threads=16;""") | ||
| try: | ||
| cursor = self.conn.execute(data_query) | ||
| yield from cursor.fetch_record_batch( | ||
| rows_per_batch=self.config.read_batch_size | ||
| ) | ||
| finally: | ||
| if self.location_scheme == "s3": | ||
| self.conn.execute( | ||
| f"""set threads={self.metadata.config.duckdb_connection_threads};""" | ||
| ) | ||
|
|
||
| def read_dataframes_iter( | ||
| self, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of the key to the new keyset pagination.
We have ordered the columns
filename_hash, run_id_hash, run_record_offsetfor the batch, so by taking the last row from the batch, we can perform our next quere where(tuple of values) > (last batch tuple of values).Because we do convert the batch results to a pandas dataframe, getting this last row from the batch is trivial.