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

Fix the behavior or column SetName method #6676

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Fix the behavior or column SetName method #6676

merged 7 commits into from
Jul 6, 2023

Conversation

asmirnov82
Copy link
Contributor

@asmirnov82 asmirnov82 commented May 14, 2023

Add references to parent DataFrames inside the column. Column renaming updates metadata of all dataframes that this column belongs to.

Fixes #6129
Fixes #5916
Fixes #6682

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #6676 (cdb2bb6) into main (184e661) will increase coverage by 0.00%.
The diff coverage is 92.15%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6676   +/-   ##
=======================================
  Coverage   68.87%   68.88%           
=======================================
  Files        1216     1216           
  Lines      250915   250958   +43     
  Branches    26259    26261    +2     
=======================================
+ Hits       172825   172865   +40     
- Misses      71265    71274    +9     
+ Partials     6825     6819    -6     
Flag Coverage Δ
Debug 68.88% <92.15%> (+<0.01%) ⬆️
production 63.37% <86.20%> (+<0.01%) ⬆️
test 88.91% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...crosoft.Data.Analysis/DataFrameColumnCollection.cs 81.06% <70.00%> (-0.56%) ⬇️
src/Microsoft.Data.Analysis/DataFrameColumn.cs 67.14% <93.75%> (+2.61%) ⬆️
src/Microsoft.Data.Analysis/DataFrame.Join.cs 94.87% <100.00%> (ø)
src/Microsoft.Data.Analysis/DataFrame.cs 86.45% <100.00%> (ø)
...st/Microsoft.Data.Analysis.Tests/DataFrameTests.cs 99.27% <100.00%> (+<0.01%) ⬆️

... and 9 files with indirect coverage changes

@michaelgsharp
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for submitting this!

@michaelgsharp
Copy link
Member

Hey @asmirnov82, looks like these changes are causing a stack overflow.

Stack overflow.
   at System.Collections.Generic.Dictionary`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].FindValue(System.__Canon)
   at System.Collections.Generic.Dictionary`2[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Item(System.__Canon)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)
   at Microsoft.Data.Analysis.DataFrameColumnCollection.UpdateColumnNameMetadata(Microsoft.Data.Analysis.DataFrameColumn, System.String)
   at Microsoft.Data.Analysis.DataFrameColumn.SetName(System.String)

Can you take another look at it? You should be able to repro locally by running the tests in Microsoft.Data.Analysis.Tests. I'm not sure which test exactly is failing since its blowing up before its able to print out the test name.

@asmirnov82
Copy link
Contributor Author

Hello @michaelgsharp, stack overflow is fixed

@JakeRadMSFT
Copy link
Contributor

Merged into Generic Math branch. (This is just a note for myself)

@JakeRadMSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@JakeRadMSFT JakeRadMSFT merged commit 53c0f26 into dotnet:main Jul 6, 2023
@asmirnov82 asmirnov82 deleted the 5916_fix_dataframe_column_renaming branch July 7, 2023 05:57
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants