mnorfolk03 commented on PR #13085:
URL: https://github.com/apache/datafusion/pull/13085#issuecomment-2435902070

   @alamb I've run cargo fmt. 
   
   > Thanks @mnorfolk03 -- this is a nice improvement
   > 
   > I ran these benchmarks and they look good.
   > 
   > ```sql
   > cargo bench --bench sql_planner -- physical_join_distinct
   > ```
   > 
   > I also made some flamegraphs to see what might be going on which was quite 
instructive:
   > 
   > ```shell
   > sudo flamegraph -- target/release/deps/sql_planner-65bd83b445d4016c 
--bench physical_join_distinct
   > ```
   > 
   > 
![flamegraph2](https://private-user-images.githubusercontent.com/490673/379750189-bd509c9a-5e7d-4209-a75e-7de28e0d83d3.svg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjk3ODk5MTEsIm5iZiI6MTcyOTc4OTYxMSwicGF0aCI6Ii80OTA2NzMvMzc5NzUwMTg5LWJkNTA5YzlhLTVlN2QtNDIwOS1hNzVlLTdkZTI4ZTBkODNkMy5zdmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDI0JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAyNFQxNzA2NTFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01MDgxNTc4ZmFlMDc2MjJkNjg4MmUxZDhmMGEwZjE0MTg0NTU0NWNhOGZlZGQyNmQ5YTUwMzQ4NjNkYTZiZjg4JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.bzqywxuSnL9piIMOVOfPQzXFga7DdElqYz-OF1nYQB8)
   > 
   > ![Screenshot 2024-10-24 at 7 35 28 
AM](https://private-user-images.githubusercontent.com/490673/379750040-edfc13b5-87ec-4663-8591-09e58b53bb85.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjk3ODk5MTEsIm5iZiI6MTcyOTc4OTYxMSwicGF0aCI6Ii80OTA2NzMvMzc5NzUwMDQwLWVkZmMxM2I1LTg3ZWMtNDY2My04NTkxLTA5ZTU4YjUzYmI4NS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDI0JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAyNFQxNzA2NTFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT01MzhhY2U0M2Y3ZjkxNTEwYTk5YzlmYmVjZTU5ZDZlYmZhZGU5YjMwMjQzMGUxYmJlNGMxZDFhMmE0NDViN2IzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Lt7oVHCn93r0EALFECkK6P3EfgJWGjS21Xr8aMmBAQQ)
   > 
   > ```shell
   > sudo flamegraph -- target/release/deps/sql_planner-65bd83b445d4016c 
--bench physical_many_self_joins
   > ```
   > 
   > 
![flamegraph3](https://private-user-images.githubusercontent.com/490673/379750844-a97f5f16-c8e2-4a17-a9b1-3a006c36b12f.svg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjk3ODk5MTEsIm5iZiI6MTcyOTc4OTYxMSwicGF0aCI6Ii80OTA2NzMvMzc5NzUwODQ0LWE5N2Y1ZjE2LWM4ZTItNGExNy1hOWIxLTNhMDA2YzM2YjEyZi5zdmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDI0JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAyNFQxNzA2NTFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jMzQwZmUxOWNhNDQ1YTAxNmI2MjhjNWNjN2U4ZTI2NTJhOTQ0YTg4NDE3ZDkzNzRhN2FjNGNhYzY2ZTQ3NDk3JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.n8DTwYAk3pEw6_7X0O2rIXoX5Q--N3D9JTpdN13WXWQ)
   > 
   > ![Screenshot 2024-10-24 at 7 37 38 
AM](https://private-user-images.githubusercontent.com/490673/379750718-aa36ee39-cb75-47db-8e5e-fed06d75c6d5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjk3ODk5MTEsIm5iZiI6MTcyOTc4OTYxMSwicGF0aCI6Ii80OTA2NzMvMzc5NzUwNzE4LWFhMzZlZTM5LWNiNzUtNDdkYi04ZTVlLWZlZDA2ZDc1YzZkNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMDI0JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTAyNFQxNzA2NTFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1mNzEzZDlkZTVhNmQ0MWFjZDFmYzI2OThkZTVjNzRmOGVkNDcxMmIzMjViODAyYmRkMGZhYzk4MWVmZmQ0NTkzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.Z0dpOAyORDbl_jqh4DxxLgCoHxTrK6dppcAHYwdR9ac)
   > 
   > So TLDR is I think these are a significant improvement to our coverage. 
Thank you 🙏
   > 
   > One thing that might be helpful to improve more would be adding a 
ParquetExec as well as queries that have sortedness
   > 
   > So the equivalent of planning against tables like this (docs here): 
https://datafusion.apache.org/user-guide/sql/ddl.html#create-external-table
   > 
   > ```sql
   > CREATE EXTERNAL TABLE foo STORED AS PARQUET LOCATION '..'
   > ```
   > 
   > ```sql
   > CREATE EXTERNAL TABLE test (
   >     c1  VARCHAR NOT NULL,
   >     c2  INT NOT NULL,
   >     c3  SMALLINT NOT NULL,
   >     c4  SMALLINT NOT NULL,
   >     c5  INT NOT NULL,
   >     c6  BIGINT NOT NULL,
   >     c7  SMALLINT NOT NULL,
   >     c8  INT NOT NULL,
   >     c9  BIGINT NOT NULL,
   >     c10 VARCHAR NOT NULL,
   >     c11 FLOAT NOT NULL,
   >     c12 DOUBLE NOT NULL,
   >     c13 VARCHAR NOT NULL
   > )
   > STORED AS CSV
   > WITH ORDER (c2 ASC, c5 + c8 DESC NULL FIRST)
   > LOCATION '/path/to/aggregate_test_100.csv'
   > OPTIONS ('has_header' 'true');
   > ```
   
   I'll leave this for a future PR since I've run out of free time this week. 
Should I open an issue for this in that case?
   
   Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to