Uh oh!
There was an error while loading. Please reload this page.
Add file_row_index UDF to query file-level row indexes from Parquet files - #22604
Conversation
file_row_index UDF to query file-level row indexes from Parquet files| let expr = &proj_expr.expr; | ||
| let col_stats = if let Some(col) = expr.downcast_ref::<Column>() { | ||
| std::mem::take(&mut stats.column_statistics[col.index()]) | ||
| column_statistics_at(&stats.column_statistics, col.index()) |
There was a problem hiding this comment.
Because I'm using Column, we currently assume it has stats. I think falling back here to unknown is reasonable.
I think a follow up here might be to craft more accurate stats depending on what created the virtual column.
There was a problem hiding this comment.
Looking at this loop again, I think that theoretically we might have multiple expression referencing a singe column, so in the current logic the second one will have unknown_stats.
| &self.signature | ||
| } | ||
| fn return_type(&self, args: &[DataType]) -> Result<DataType> { |
There was a problem hiding this comment.
I looked at UuidFunc that returns a plain Utf8 instead of the arrow extension type through return_field_from_args
0227cd9 to
4ce49feCompare1d1c61e to
e33fe3fCompare
alamb
left a comment
There was a problem hiding this comment.
Thanks @AdamGS -- this looks great to me
The only thing I think we should add (can be a follow on PR) is a test when the source doesn't support file_row_index()
Also, do you plan to add something like file_url() or something like that to distinguish between the different input files?
cc @mbutrovich and @adriangb
I played around a bit with it locally and it works great:
DataFusion CLI v53.1.0>selectcount(*) from'/Users/andrewlamb/Software/datafusion/benchmarks/data/hits.parquet';
+----------+
| count(*) |
+----------+
| 99997497 |
+----------+1 row(s) fetched.
Elapsed 0.015 seconds.
>selectmin(file_row_index()) from'/Users/andrewlamb/Software/datafusion/benchmarks/data/hits.parquet';
+-----------------------+
| min(file_row_index()) |
+-----------------------+
| 0 |
+-----------------------+1 row(s) fetched.
Elapsed 0.009 seconds.
>selectmax(file_row_index()) from'/Users/andrewlamb/Software/datafusion/benchmarks/data/hits.parquet';
+-----------------------+
| max(file_row_index()) |
+-----------------------+
| 99997496 |
+-----------------------+1 row(s) fetched.
Elapsed 0.007 seconds.
>selectcount(distinct file_row_index()), count(*) from'/Users/andrewlamb/Software/datafusion/benchmarks/data/hits.parquet';
+----------------------------------+----------+
| count(DISTINCT file_row_index()) | count(*) |
+----------------------------------+----------+
| 99997497 | 99997497 |
+----------------------------------+----------+1 row(s) fetched.
Elapsed 1.727 seconds.It is also pretty cool to aggregate the row index:
>select"URL", array_agg(file_row_index()), count(*) from'/Users/andrewlamb/Software/datafusion/benchmarks/data/hits.parquet'GROUP BY"URL"HAVINGcount(*) >5ORDER BYcount(*) limit10;
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------+----------+
| URL | array_agg(file_row_index()) | count(*) |
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------+----------+
| http://smeshariki.ru/index.ua/search.jsp#firstyle=0&category=cinema/movie/kruzhok-gorod.irr.ru/catalog/content/Dom-Zaporia.ua/?target%3D43%26bt%3D43%26bt%3D90707 | [42666365, 42666366, 42666367, 42666368, 42666369, 42666370] | 6 |
| http://smeshariki.ru/index.ua/search/?target=search?typ=POR&anbietersburg.irr.ru/regislavl.irr.ru/GameMain.aspx | [42662761, 42662762, 42662764, 44461467, 65634145, 65868576] | 6 |
| http://smeshariki.ru/index.ru/proskategory_name=Покупки в дома/БСЭ/Пятый - bpc bonprix | [42665535, 65699565, 65699566, 65704608, 65852397, 87666422] | 6 |
| http://smeshariki.ru/superchart/516728162&pt=b&pd=9&pw=2&pv=23&prr=http://afisha.yandex.ru/mymail.ru/film/45332320687670014823900941.html_params%3Drhost%3D90%26nid%3D278885%26bt%3Dad.adrive_type_id=266166629%26sliceid%3D0%26rnd%3D278885 | [42656519, 77023348, 65849789, 76820757, 87928180, 88010383] | 6 |
| http://smeshariki.ru/index.ua/search.aspx#locationcoding/secondary/detail/5544158197%26width%3Dhttp://karticle | [42660711, 65971477, 41318773, 41874303, 41874305, 42423747] | 6 |
| http://smeshariki.ru/a-folders/uF77289738373707475dfc312-160/guest-id=3630394&ch=UTF-8&sF=0,0,224.jpg%26rleurl%3D%26CompPath%3D278885 | [42651017, 43509816, 66042296, 87818689, 41404533, 42621614] | 6 |
| http://smeshariki.ru/index.ua/search_type_id=3869&pvno=2&evlg=VC,3;VL,30643256.html?1=1&cid=577&oki=1&op_uid=1515.html%26custom%3D%26xpid%3D1%26version/vault.ru/linkvac.php4?newpower_name=Купальный -bonprix.ru/GameMain.aspx | [42654955, 43676909, 88235766, 41099784, 41450355, 41732137] | 6 |
| http://smeshariki.ru/index.ua/advanceForm[district=&srgender_by=0&hide_black_list.html%3Fhtml?html&server/298596701534][to]=&int[537][to]=&int[858][from]=&price=False&pNum=30623839455be50653338 | [42651118, 43201053, 65725681, 87441478, 41318997, 42113884] | 6 |
| http://smeshariki.ru/recipes/default-Medvezhestva | [42651157, 42651158, 44116167, 65488371, 42044907, 42538449] | 6 |
| http://smeshariki.ru/index.ua/search/?target=search?typ=SEM_BR&aktions[0][153]=0-0-44487%26width%3Dhttp://smeshariki.ru/search?text=фильм на руссказки | [42667401, 65911944, 77554747, 76831140, 41082598, 41240380] | 6 |
+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+--------------------------------------------------------------+----------+| return Ok(recursion); | ||
| } | ||
| if ScalarFunctionExpr::try_downcast_func::<FileRowIndexFunc>(node.as_ref()) |
There was a problem hiding this comment.
I am surprised that marking the function volatile isn't sufficient to prevent pushdown 🤔
There was a problem hiding this comment.
I'm pretty sure an earlier version ran into that even with ExpressionPlacement, not sure what changed.
| /// The rewrite matches the concrete [`ScalarUDFImpl`] type rather than the | ||
| /// function name. `replacement` is called with each matching | ||
| /// [`ScalarFunctionExpr`] after its children have been rewritten. | ||
| pub fn rewrite_scalar_udf<T, F>( |
There was a problem hiding this comment.
why make this a public API? It seems fine to me, but somewhat random
There was a problem hiding this comment.
My thinking was that I keep seeing people mess some of the scalar_udf handling and this might make it nicer, but I'll make it private for now.
| tree_node::{Transformed, TransformedResult, TreeNode, TreeNodeRecursion}, | ||
| }; | ||
| use datafusion_expr::ScalarUDFImpl; | ||
| use datafusion_functions::core::{ |
There was a problem hiding this comment.
It is unfortunate to me that the physical expr has to special case FileRowIndexFunc, but sort of some very specific API on PhysicalExpr (like is_row_index_func()) I can't really think of anything better.
I realize we already have the special case for GetFieldFunc so I think this is ok
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| statement ok |
There was a problem hiding this comment.
Can you also add a test for how this function works for some source that doesn't support the file_row_index()? According to me testing it seems like it just returns NULL:
>select*, file_row_index() from'./datafusion-examples/data/csv/cars.csv';
+-------+-------+---------------------+------------------+
| car | speed | time | file_row_index() |
+-------+-------+---------------------+------------------+
| red | 20.0 | 1996-04-12T12:05:03 | NULL |
| red | 20.3 | 1996-04-12T12:05:04 | NULL |
...
| green | 8.0 | 1996-04-12T12:05:13 | NULL |
| green | 2.0 | 1996-04-12T12:05:14 | NULL |
+-------+-------+---------------------+------------------+25 row(s) fetched.
Elapsed 0.002 seconds.What do you think about returning a runtime error if it is evaluated instead of NULL?
There was a problem hiding this comment.
Definitely a reasonable option, probably a lot easier to debug than getting a NULL inside a complex query that causes unexpected results.
There was a problem hiding this comment.
added a SLT test over CSV
| +-----+ | ||
| ``` | ||
| ### `file_row_index` |
There was a problem hiding this comment.
We discussed the actual API in #20135
The alternate we discussed was some sort of _metadata virtual column
However, it seems like the final decision (per @adriangb was #20135 (comment))
IIUC the plan we end up with is:
FilterExec: projection=[other_col], filter=[file_row_index() > 3] DataSourceExec: projection=[other_col]
Which seems reasonable to me. I didn't find any common API across implementations. See comment here
| let mut source = self.clone(); | ||
| source.projection = self.projection.try_merge(projection)?; | ||
| if !projection.iter().any(|projection_expr| { |
There was a problem hiding this comment.
Can you add a comment about why the merge is skipped if there is a file_row_index() call?
| } | ||
| } | ||
| fn table_schema_with_row_index_col(table_schema: &TableSchema) -> (TableSchema, Column) { |
There was a problem hiding this comment.
some comments here would help future readers I think
There was a problem hiding this comment.
Its now Projection_exprs::projected_column_position and returns Option<usize>.
Uh oh!
There was an error while loading. Please reload this page.
| ProjectionExprs::new(base_exprs).try_merge(&rewritten_projection) | ||
| } | ||
| fn row_index_projection_idx( |
There was a problem hiding this comment.
This seems like a good method to add to ProjectionExprs (and add some documentation) as it could potentially be used by others
I'll address everything by next week, as file the file URL, did you have in mind something different from |
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
alamb
commented
Jun 11, 2026
I think #22604 is this pR Perhaps you meant Yes that looks good. I'll make some comments |
5d41a2d to
f0e89d4CompareSigned-off-by: Adam Gutglick <adamgsal@gmail.com>
f0e89d4 to
6998172CompareAdamGS
commented
Jun 11, 2026
I think I addressed all comments, I apologize for the git noise did a mindless rebase and had to fix things. |
mbutrovich
commented
Jun 11, 2026
Have any sharp edges/API mismatches come out of #22026? We haven't cut a release with that change in, so happy to hear any API feedback and/or accept changes here if need be. |
AdamGS
commented
Jun 11, 2026
Not that I can remember, and the only thing that jumps out to me is the re-writing of the |
AdamGS
commented
Jun 15, 2026
I'm going to start on #6051, I think I'll build it on top of this branch because some of the machinery I added seems useful for the rewrite path. |
alamb
left a comment
There was a problem hiding this comment.
looks good to me -- thank you @AdamGS and @mbutrovich
| STORED AS CSV | ||
| LOCATION 'test_files/scratch/file_row_index/csv_table/data.csv'; | ||
| query error file_row_index\(\) is source dependent and cannot be evaluated directly |
Uh oh!
There was an error while loading. Please reload this page.
AdamGS
commented
Jun 16, 2026
I don't see an issue in the merge queue, what am I missing here? |
2010YOUY01
commented
Jun 16, 2026
I guess GitHub runners are flaky lately |
Which issue does this PR close?
Rationale for this change
This PR includes the "front end" side of @mbutrovich's #22026, bridging the last mile to allow users to query file row indexes.
What changes are included in this PR?
file_row_index, following feat: add input_file_name() for file-backed scans (plumbing PR) #20071's example. The function returns 0-based row indexes for Parquet scans.schema_rewriter.rswhich was the best place I could think of, but maybe they should be move elsewhere.file_row_index, which turns it into aCast(Column(...)), which is required to return Int64 values.ParquetSource::try_pushdown_projection, we look forFileRowIndexFunc, and if it exists we rewrite it and the source's table schema.Are these changes tested?
In addition to individual unit tests, I've added a new SLT file (
file_row_index.slt) that tests for the following cases:file_row_indexfrom a table backed by multiple filesfile_row_indexwhen its part of the projectionfile_row_indexwhen its not of the projection, when filter pushdown is either enabled or disabled (this part didn't work in a previous iteration, but figured it out today).Are there any user-facing changes?
FileRowIndexFunc/file_row_index,physical-expr-adapter-rewrite_file_row_index_exprspecifically for the new UDF,rewrite_file_row_index_projectionto rewrite theProjectionExprsand two utility functions that should make it clearer how to manipulate and find ScalarUDFs in physical expressions -expr_references_scalar_udfandrewrite_scalar_udf.